-
Notifications
You must be signed in to change notification settings - Fork 106
Add slice_time and slice_index arguments to IMAS geometry loader
#1416
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
Conversation
2422ada to
6632990
Compare
|
@MateoBell what's the provenance of the |
e939fb9 to
678f7cd
Compare
|
Linked to #1391. During investigating the different geometry loaders in ST, I am comparing eqdsk from various timesteps with CHEASE, IMAS, etc. Hence, I need to be able to load different slices from the IMAS equilibrium IDS. |
The .nc file was generated with chease converting the EQDSK_ITERhybrid_COCOS02.eqdsk (actually not completely sure which eqdsk of the examples, but it is the one which was used to compare to the CHEASE default geometry file). There is a script to run CHEASE from an eqdsk and save it to an IDS in a database, and then I converted it in netCDF. The script probably requires IMAS core so I can try to generate an IDS with multiple time slices with given eqdsk if needed. |
|
Thanks! I think for this PR we ideally would have an IDS with different profiles across the time steps we're comparing. This will also pave the way for smoothly interpolating between geometries to have evolving geometry over the run, but this may require rerunning CHEASE (or equivalent fixed-boundary solver) to produce something consistent. We could, for example, run CHEASE twice using the pprime and FFprime profiles from the beginning and end of the TORAX iterhybrid rampup case as inputs, to give two different geometries for the start and end of the ramp. What do you think? |
Yes fully agree ! I think it is a good idea so we can get different geometries at different times to compare. |
|
I've never run CHEASE before, if you're able would you be able to generate the two geometries? Failing CI is just the placeholder test that's waiting for the new data. |
678f7cd to
40fd9cd
Compare
|
Ok I think I should be able to do it, will try to do it asap ! |
|
Thanks both! Having a multiple time-slice IDS loader and a test for it would be ideal |
|
I added the multiple time slices IDS, but I just noticed the netCDF file is pretty heavy. I generated it with CHEASE for all time slices of the output simulation from iterhybrid_rampup config. If needed I can reduce the number of time slices to reduce the weight. |
4b4b771 to
6c9e5bd
Compare
|
Incredible, thanks so much @MateoBell. Your fixes to the tests also seem fine! In the latest force push I just rebased onto main. For reference, the full .nc is 56.21 MB (GitHub's recommended limit is 50MB). I think we can definitely reduce the number of timeslices - maybe we should go down to ~10 from ~40? |
|
Thanks both!
Yes, good idea to not load up the repo with large files |
|
Okay just replaced it with 11 time slices evenly spaced in time, the file is now ~16MB ! |
4234aa1 to
95a7fde
Compare
Nush395
left a comment
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.
thanks Theo! Left a few comments
torax/_src/geometry/imas.py
Outdated
| if slice_time is not None: | ||
| # Find the closest time in the IDS that is <= slice_time | ||
| slice_index = ( | ||
| np.searchsorted(equilibrium.time, slice_time, side="right") - 1 |
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.
is equilibrium.time guaranteed to be sorted?
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.
Unsure. Happy to add a check and then sort if it's not.
|
@theo-brown , sorry this one slipped through the cracks. If it's still relevant can you respond to any comments, sync, and we'll re-review? |
95a7fde to
3cccd78
Compare
|
No problem, it also slipped off my radar! I didn't see @Nush395 had reviewed it. Changes made! I wasn't sure whether it's ok to assume time is sorted in the equilibrium IDS, I'm not sure whether this is enforced. Can easily add a check and do a sort-then-search if you think it's required. |
Nush395
left a comment
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.
thanks Theo! Looks great
3cccd78 to
e32aa42
Compare
e32aa42 to
51b30bf
Compare
Added an IDS with multiple time slices for testing
51b30bf to
5d2093b
Compare
|
Thanks for the review @Nush395! Changes:
|
| f"{len(equilibrium.time_slice)} time slices" | ||
| ) | ||
| IMAS_data = equilibrium.time_slice[slice_index] | ||
| B_0 = np.abs(IMAS_data.global_quantities.magnetic_axis.b_field_phi) |
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 just noticed this additional unused B_0, should this be removed? If so I can do so.
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.
Yeah this is definitely wrong, not sure why I added it 🤔 please remove!
|
@Nush395 I think something might've gone wrong in the copybara import. From the merge commit: 2e5f70e torax/torax/_src/geometry/pydantic_model.py Line 314 in 2e5f70e
From the branch: 5d2093b torax/torax/_src/geometry/pydantic_model.py Line 314 in 5d2093b
|
@jcitrin commented in internal review that this jcitrin: "time can be negative, it's a user definition. There are use-cases where t=0 is some "important reference time" in the middle of a simulation" |
|
Ok, cool! Thanks for clarifying :) |
This enables loading an equilibrium from any timeslice in an equilibrium IDS. Useful if resuming from pre-existing simulations.