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

Levels #21

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Levels #21

wants to merge 19 commits into from

Conversation

Geet-George
Copy link
Collaborator

@Geet-George Geet-George commented Jul 29, 2020

Template function to estimate values at given atmospheric level and given definition of said level.

@leifdenby
Copy link
Collaborator

This is looking really cool @Geet-George! Do you want to make talk us through how you see it working? We could do that after the "kitchen-meetup" today 😄

@JuleRadtke
Copy link
Collaborator

Ja really great Geet! We can go through it and distribute the missing parts to get it filled!?

@Geet-George
Copy link
Collaborator Author

This is looking really cool @Geet-George! Do you want to make talk us through how you see it working? We could do that after the "kitchen-meetup" today 😄

Sure Leif! I can do that...

@Geet-George Geet-George marked this pull request as draft July 30, 2020 09:55
@Geet-George
Copy link
Collaborator Author

Ja really great Geet! We can go through it and distribute the missing parts to get it filled!?

Yep... After the kitchen meeting and our group's meeting!?

@Geet-George Geet-George marked this pull request as ready for review July 30, 2020 16:01
@Geet-George Geet-George requested a review from leifdenby July 31, 2020 09:36
@Geet-George Geet-George requested review from annalea-albright, leifdenby and JuleRadtke and removed request for leifdenby July 31, 2020 09:36
Copy link
Collaborator

@leifdenby leifdenby left a comment

Choose a reason for hiding this comment

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

This looks great @Geet-George! The only thing I'd suggest is to split the code into more individual functions:

  1. pass in dataset -> return the dataset "annotated" so that there is a new variable (say layer) with dimensions of the underlying dataset that describes which layer each point (in space and time) belongs to. Having this functionality lets the user do .groupby("layer").mean() operations them selves and get the values out. This also makes it possible to do plots like ds.rh.plot(hue="layer") for example

  2. make the plotting function use this annotated dataset, I think that should mean you could avoid the eval(...) calls you have :)

@Geet-George
Copy link
Collaborator Author

Geet-George commented Jul 31, 2020

Oh yeah.. An annotated dataset makes so much more sense. I'll add this functionality and this will also let me avoid using eval(), which I was really not happy with. Thanks!

@leifdenby
Copy link
Collaborator

@Geet-George should we just merge this? Then I can take a stab at making it work with the changes in #30

@leifdenby
Copy link
Collaborator

Or are you working on the changes we talked about above?

@Geet-George
Copy link
Collaborator Author

This is unfinished... I'll probably change it quite a bit, but won't be able to do it before next week. Could we wait until then before merging? I'll make sure changes are compatible with #30

@leifdenby
Copy link
Collaborator

This is unfinished... I'll probably change it quite a bit, but won't be able to do it before next week. Could we wait until then before merging? I'll make sure changes are compatible with #30

Yes, great. Let's leave it for now, just wanted to check :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants