Skip to content
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

Closed
wants to merge 21 commits into from
Closed

Conversation

ninarobbins
Copy link
Contributor

Add the following variables to Level 4:

"div",
"vor",
"mean_density",
"w_vel",
"omega"

all of which have dimensions (circle, alt)

@ninarobbins ninarobbins changed the title Lev4 add products Add Level 4 products Sep 23, 2024
@ninarobbins ninarobbins marked this pull request as ready for review September 23, 2024 19:47
@ninarobbins
Copy link
Contributor Author

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

@hgloeckner
Copy link
Contributor

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

Copy link
Contributor

@hgloeckner hgloeckner left a 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

@@ -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):
Copy link
Contributor

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

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))
Copy link
Contributor

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

self.circle_ds = self.circle_ds.assign(
dict(density=(self.circle_ds.ta.dims, density.magnitude))
)
self.circle_ds["density"].attrs = {
Copy link
Contributor

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))
        )

self.circle_ds = self.circle_ds.assign(
dict(mean_density=self.circle_ds["density"].mean(sonde_dim))
)
self.circle_ds["mean_density"].attrs = {
Copy link
Contributor

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


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 = {
Copy link
Contributor

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(
Copy link
Contributor

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

@Geet-George
Copy link
Contributor

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.

@ninarobbins
Copy link
Contributor Author

Thanks @hgloeckner and @Geet-George for the comments. Step by step I am implementing them :)

height = xr.concat([zero_vel, div["alt"]], dim="alt")
height_diff = height.diff(dim="alt")

del_w = -div * height_diff.values
Copy link

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

Copy link
Contributor

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.

Copy link
Contributor

@Geet-George Geet-George Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, just noticed some links there need to be updated (see issue #58). but these should be fixed as soon as #74 is merged.

Copy link
Contributor

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.

Copy link

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 !

Copy link
Contributor Author

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?

Copy link

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 ?

@hgloeckner hgloeckner mentioned this pull request Dec 13, 2024
@hgloeckner hgloeckner closed this Dec 13, 2024
@hgloeckner hgloeckner mentioned this pull request Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants