Skip to content

Removing fieldtype argument from Field constructor#1942

Merged
erikvansebille merged 3 commits intov4-devfrom
removing_fieldtype_argument
Mar 25, 2025
Merged

Removing fieldtype argument from Field constructor#1942
erikvansebille merged 3 commits intov4-devfrom
removing_fieldtype_argument

Conversation

@erikvansebille
Copy link
Copy Markdown
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
Copy Markdown
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.

@github-project-automation github-project-automation bot moved this from In review to Ready in Parcels v4 release Mar 25, 2025
@github-project-automation github-project-automation bot moved this from Backlog to Ready in Parcels development Mar 25, 2025
@VeckoTheGecko
Copy link
Copy Markdown
Contributor

I also added a setter for Field.units

@erikvansebille
Copy link
Copy Markdown
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
@github-project-automation github-project-automation bot moved this from Ready to Done in Parcels v4 release Mar 25, 2025
@github-project-automation github-project-automation bot moved this from Ready to Done in Parcels development Mar 25, 2025
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