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
netCDF Front End for Cloud Model Simulations #2176
netCDF Front End for Cloud Model Simulations #2176
Changes from 52 commits
0162faf
58126a4
922a4e6
de1b58a
30e6253
5847686
48505d0
c9d48dd
66b4e03
378c0b4
42b6abf
4106814
15ac1bf
7fe75e9
96306e8
dc181b2
df98c1f
11b8c9a
e0bcc5d
306cf09
c448a7b
58149e1
557621b
04aeb6c
b58ead2
0fc8367
bd5abb5
f3dff05
e1914b3
1dc8694
f13bf04
fa1ee32
214de7f
e19de77
48aed38
3f49b4c
2f32253
f20ab6b
2ea2cdd
cc4f460
fe74465
4ed2d23
991497a
d90a8b2
dfea412
80c6a20
eef55ac
dec3f0b
860fa19
5c423ba
0932088
1aada12
11116a2
58bca15
53e6162
689cdf6
12166da
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.
I can't help but notice that in the AMRVAC frontend this method seems to do much less
Note that the Athena frontend for example is closer to AMRVAC than to this.
I'm wondering which one is wrong.
@matthewturk ?
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.
@matthewturk can you weigh in here please ?
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.
So it depends. The reason we ever do
self.grids[i]
is if we are creating an object array in advance. I think it might also no longer be necessary -- there was a segfault that we ran into many years ago as a result of numpy object arrays, which is perhaps no longer operative.But the idea was, create an empty numpy object array, then fill it. It's possible that in AMRVAC it creates the grid objects elsewhere and they are already populated.
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.
Are
xh
,yh
,zh
the cell centers or the cell edges?I would naively expect that if
xh
represents the cell edges, then the number of dimensions isds.dimensions["xk"].size - 1
, and if it represents the cell centers, then the left and right edge should be computed by adding-dx/2
and+dx/2
respectively.I however see that you are using
ds.dimensions
andds.variables
, so there is definitely some subtlety that I am missing. In any case, it'd be worth commenting to say what thexh
(as well asyh
andzh
represent).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.
we'll need @keltonhalbert to weigh in on
xh
,yh
andzh
(I think they are actually cell centers but I'm not positive?). I think adding a comment to the code would be good here -- maybe including a link to thecm1_lofs
docs website?But I think some confusion is coming from the fact that
ds
here is our netcdf dataset file handle, not ayt
dataset. Sods.variables
contains the netcdf objects with the actual data that need to be parsed and read whileds.dimensions
is a netcdf attribute with dimensionality metadata. I think this will be clearer by switchingds
to a different name (_handle
as Matt suggested).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.
@chrishavlin @cphyc xh, yh, and zh are the cell center coordinates where scalars are defined.
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.
added to the comments around this section noting the xh, yh and zh coordinates.
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 it always the case? Or could you imagine cloud models produced with your code with periodic boundary conditions? If this is likely, add a
# TODO
comment to say that periodicity should be supported, or even better (if possible) raise aNotImplementedError
if periodicity is detected.If no cloud model uses periodicity, then would you mind adding a comment to document it? Something like