Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ensure correct data types in getter methods #1030
ensure correct data types in getter methods #1030
Changes from 19 commits
5750442
b017c84
3a09829
373a8bb
c5467c6
b3684de
d4e6f56
2f14489
3b8d21a
02aa099
63b66af
af6ba1e
fccf6fe
4a1dd9c
ed7beb1
a4a80cf
0c8c20f
49a4b63
4aa319b
349ea40
0b0588f
80c40ab
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While not directly related to the underlying issue for this PR, a lot of the data type errors were coming from the
_CoilObjective.build
method. The pytree stuff we had in here was really clunky, like creating aMixedCoilSet
that contained_Grid
s instead of_Coil
s. I tried to simplify the logic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked at it a little bit but haven't gotten the chance to understand the changes yet
Check warning on line 125 in desc/objectives/_coils.py
Codecov / codecov/patch
desc/objectives/_coils.py#L125
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might want to make this a
ValueError
with a more descriptive error message?Or maybe put a separate check after
if isinstance(grid, list):
since that's the only case where it could cause problemsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also what if the user passes in different grids for each coil in a regular
CoilSet
? I think in that case the code as is now will only use the first grid?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re your second question: yes the code will only use the first grid. That is how the existing code also behaves, so this PR is not changing that. My changes earlier this week corrected this, but then I reverted them because my solution didn't work for nested coilsets. We could try to fix this in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor point: This will get more transforms than needed, since the ones for
CoilSet
are later pruned to just the unique ones. Would be nice if we can avoid that redundant calculation but not sure how feasible it is.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've resolved this in the latest commit