-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add Level 4 products #60
Conversation
tests are failing due to no circle_fit module, even though I added it to the environment.yml... I don't quite get the issue, it works locally |
Yes, with the change to poetry (to make this a pip package), adding dependencies to the environment.yaml is not enough. We added a section to the Contributing section of the documentation You have to update the pyproject.toml and the poetry.lock |
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.
Looks good :) except for some minor things; mainly removing the things that are already in the other pull request and cleaning up the attributes and their assignment
pydropsonde/circles.py
Outdated
@@ -15,6 +23,172 @@ class Circle: | |||
platform_id: str | |||
segment_id: str | |||
|
|||
def dummy_circle_function(self): | |||
print(self.flight_id, self.segment_id) | |||
def get_xy_coords_for_circles(self): |
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.
the changes that are already part of another pull request should not be part of this one as well
pydropsonde/circles.py
Outdated
vor = self.circle_ds.dvdx - self.circle_ds.dudy | ||
|
||
self.circle_ds = self.circle_ds.assign( | ||
dict(div=(["alt"], D.values), vor=(["alt"], vor.values)) |
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.
maybe it would be good to add attributes here as well, explaining what the variables are and what unit they have
pydropsonde/circles.py
Outdated
self.circle_ds = self.circle_ds.assign( | ||
dict(density=(self.circle_ds.ta.dims, density.magnitude)) | ||
) | ||
self.circle_ds["density"].attrs = { |
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 learned from tobi during this campaign, that [] should not come before a = with xarray; you can assign the attributes in the ds.assign call above, like
self.circle_ds = self.circle_ds.assign(
dict(density=(self.circle_ds.ta.dims, density.magnitude, density_attrs))
)
pydropsonde/circles.py
Outdated
self.circle_ds = self.circle_ds.assign( | ||
dict(mean_density=self.circle_ds["density"].mean(sonde_dim)) | ||
) | ||
self.circle_ds["mean_density"].attrs = { |
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.
see above;
also, as discussed in other pull requests/issues "standard_name" should only contain names from the cf conventions; maybe you can look for the right ones and add those instead? in the first draft I just added some description, but that should be made better in the actual package
pydropsonde/circles.py
Outdated
|
||
w_vel = del_w.cumsum(dim="alt") | ||
self.circle_ds = self.circle_ds.assign(dict(w_vel=w_vel)) | ||
self.circle_ds["w_vel"].attrs = { |
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.
see above
* units(self.circle_ds.w_vel.attrs["units"]) | ||
* mpconst.earth_gravity | ||
) | ||
self.circle_ds = self.circle_ds.assign( |
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.
maybe here again, assigning attributes to the data variable would be good
I just remembered that there is a bug in estimating omega, so the JOANNE function needs to change a bit. See issue here and relevant pull request here. |
2c26fe5
to
da5c37a
Compare
Thanks @hgloeckner and @Geet-George for the comments. Step by step I am implementing them :) |
dae4ed4
to
272106c
Compare
pydropsonde/circles.py
Outdated
height = xr.concat([zero_vel, div["alt"]], dim="alt") | ||
height_diff = height.diff(dim="alt") | ||
|
||
del_w = -div * height_diff.values |
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.
Hi all, wow everything seems to have advanced so much already ! Not sure to where leave my comment but there is a problem in this definition of vertical velocity because it assumes air is incompressible. Instead omega should be computed first by integrating divergence over pressure (which accounts for air compressibility), and then vertical velocity can be computed by dividing by rho*g. Is that clear ? I can update those two functions if you want as I have already done it for the JOANNE dataset, but I am very unfamiliar with GitHub and don't know where to do this
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.
Hi @bpoujol you are the best person to help out with this indeed! :) I think the contributing section might help you get started on how you can contribute, but if you have any questions you can always ask here.
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.
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 so much for chimming in @bpoujol
Geet already pointed us to you PR for JOANNE and I am sure, @ninarobbins will change this functions accordingly.
Another way would be for you to make a PR to her repo or we could remove omega and vertical velocity from this PR and make another one for them.
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.
Thank you Geet for the documentation, very useful ! For Helene and Nina : it is as you want. If Nina plans to do it then it is great ! But the function in my JOANNE PR looks quite complicated and I am sure it is possible to make it simpler. Else, if you prefer, I can either do a PR to the repo of @ninarobbins or I wait for this PR to be incorporated in the main code before I do a second PR. Let me know what is the best !
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.
Dear @bpoujol, sorry for the super late reply, I just got back to working on this. I am happy if you do a PR to my repository, as I would prefer not to incorporate this when it is wrong. Would you still be able to 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.
Hi @ninarobbins , sorry for the late reply as well. I am very busy right now and probably still for the coming one or two weeks, is it fine if I only do this afterwards ?
272106c
to
a39950f
Compare
8019242
to
43eb4a4
Compare
Add the following variables to Level 4:
"div",
"vor",
"mean_density",
"w_vel",
"omega"
all of which have dimensions (circle, alt)