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

Removing fieldtype argument from Field constructor #1942

Merged
merged 3 commits into from
Mar 25, 2025

Conversation

erikvansebille
Copy link
Member

Removing fieldtype argument from Field constructor, as it's more pythonic to attach UnitConverters directly to a field?

See also #1883 (comment)

  • Chose the correct base branch (main for v3 changes, v4-dev for v4 changes)

As it's more pythonic to attach UnitConverters directly to a field?

See also #1883 (comment)
Copy link
Contributor

@VeckoTheGecko VeckoTheGecko left a comment

Choose a reason for hiding this comment

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

Minor edits, otherwise LGTM.

Should we be concerned with the naming of

UWind.units = parcels.tools.converters.GeographicPolar()
VWind.units = parcels.tools.converters.Geographic()

? Previously it might have been easier to identify that VWind would be of field type V, whereas now its probably not as clear to the user that it needs a Geographic converter and not a GeographicPolar converter. Perhaps this would just be something to keep an eye on moving forward, that we can revisit the naming/how to specify these unit converters.

@VeckoTheGecko
Copy link
Contributor

I also added a setter for Field.units

@erikvansebille
Copy link
Member Author

Minor edits, otherwise LGTM.

Should we be concerned with the naming of

UWind.units = parcels.tools.converters.GeographicPolar()
VWind.units = parcels.tools.converters.Geographic()

? Previously it might have been easier to identify that VWind would be of field type V, whereas now its probably not as clear to the user that it needs a Geographic converter and not a GeographicPolar converter. Perhaps this would just be something to keep an eye on moving forward, that we can revisit the naming/how to specify these unit converters.

Good point that these names may be confusing. Maybe the names DegreeToMeterLongitude and DegreeToMeterLatitude make more sense?

@erikvansebille erikvansebille merged commit 35b59a4 into v4-dev Mar 25, 2025
16 checks passed
@erikvansebille erikvansebille deleted the removing_fieldtype_argument branch March 25, 2025 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants