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

Remove deprecations, remove crs, rename ARCOERA5 #252

Merged
merged 14 commits into from
Oct 8, 2024

Conversation

zebengberg
Copy link
Contributor

@zebengberg zebengberg commented Oct 7, 2024

Closes #180
Closes #151
Closes #238

Breaking changes

  • Remove attrs["crs"] usage from GeoVectorDataset and child classes (Flight, Fleet). All spatial data is assumed to be EPSG:4326 (WGS84). This was previously assumed implicitly, but now the crs attribute is removed from the attrs dictionary.
  • Remove deprecated MetDataset.variables property in favor of MetDataset.indexes.
  • Remove **kwargs in MetDataArray constructor.
  • Rename ARCOERA5 to ERA5ARCO for consistency with the ERA5 and ERA5ModelLevel interfaces.

Features

  • Implement PSFlight.eval on a Fleet source.

Tests

  • QC test passes locally (make test)
  • CI tests pass

Reviewer

Select @ reviewer

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@zebengberg zebengberg requested a review from thabbott October 7, 2024 19:51
@zebengberg zebengberg changed the title Deprecation/arco era5 Remove deprecations, remove crs, rename ARCOERA5 Oct 7, 2024
Copy link
Contributor

@mlshapiro mlshapiro 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 good @zebengberg! Thanks for taking the time to do the cleanup.

fleet["air_temperature"] = fleet.T_isa()

ps_model = ps.PSFlight(fill_low_altitude_with_zero_wind=True)
out = ps_model.eval(fleet)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a more generic Fleet question:

I had a question here recently about how Fleet objects handle attrs. Would we be able to have a test here the shows how the attrs show up in "out"?

I would expect to find aircraft_type somewhere in the output.

assert "flight_id" in out.data and "aircraft_type" in out.data

or is there another place these attrs get stored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's not a great way to handle attrs on Fleet instances. The current paradigm is to include global properties in Fleet.attrs and individual flight properties in Fleet.fl_attrs. I expanded the test a bit to check these out: 5602a03

We haven't made consistent use of Fleet.attrs throughout pycontrails. I think the best practice would be to check for a scalar quantity in Fleet.attrs first, then to look within Fleet.fl_attrs next ... you can see how this gets complicated tricky.

Copy link
Contributor

@mlshapiro mlshapiro Oct 8, 2024

Choose a reason for hiding this comment

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

oh great! this answers my question.

Is there any reason not to use Fleet.attrs instead of Fleet.fl_attrs? Was it a compatibility or serialization question?

Copy link
Contributor

@mlshapiro mlshapiro Oct 8, 2024

Choose a reason for hiding this comment

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

(Looks like Flight.attrs have type dict[str, Any] so could handle a dict[str, dict[str, Any]])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We definitely could have used Fleet.attrs instead. We always end up creating a dictionary of the form flight_id: fl.attrs for each fl in fleet, and we make use of it in various Fleet methods, so I attached it as an explicit property on the instance rather than assuming fleet.attrs included separate keys for each flight_id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay -- so you're suggesting we overload Fleet.attrs to hold {flight_id: flight_properties}. I think could be a reasonable way to proceed. I think I avoided this in the past because attrs["crs"] was getting in my way.

I think I'll hold off for now, but may do this whenever I iterate again on Fleet.

@zebengberg zebengberg merged commit 200faee into main Oct 8, 2024
16 checks passed
@zebengberg zebengberg deleted the deprecation/arco-era5 branch October 8, 2024 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants