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

Terminology correction: alt -> height #113

Merged
merged 5 commits into from
Apr 10, 2024
Merged

Terminology correction: alt -> height #113

merged 5 commits into from
Apr 10, 2024

Conversation

fpavogt
Copy link
Member

@fpavogt fpavogt commented Apr 8, 2024

Description:

This PR implements a series of cosmetic changes to use height instead of altitude throughout the code. This is to reflect the fact that ceilometer hits and MSA are/must be both specified in ft above aerodrome level.

Although these changes are cosmetic, they will impact the end users, as the naming convention for the input Pandas DataFrame has changed, with the alt column now being height.

No version changes is included in this PR, but one should definitely raise it before making a new release to reflect this backward-incompatibly evolution.

In addition, this PR includes two additional minor changes:

  • the default example dataset now is 900s long (instead of 1200s)
  • the parameter reset function now allows to only reset a subset of parameters is required
  • minor tweaks to setup.py, to take into account the fact that setuptools must be explicitly requested as an extras package for dev work as of Python 3.12

Error(s) fixed:

Checklists:

  • New code includes dedicated tests.
  • New code has been linted.
  • New code follows the project's style.
  • New code is compatible with the 3-Clause BSD license.
  • CHANGELOG has been updated.
  • AUTHORS has been updated.
  • Copyright years in module docstrings have been updated.

@fpavogt fpavogt requested a review from regDaniel April 8, 2024 13:44
@fpavogt fpavogt self-assigned this Apr 8, 2024
@fpavogt fpavogt marked this pull request as ready for review April 8, 2024 14:09
@fpavogt
Copy link
Member Author

fpavogt commented Apr 8, 2024

Hey @regDaniel,

Here is the PR as discussed. I'll let you decide whether you prefer to merge it right away into develop, into your own branch, or delay the merge until your NSC update has been implemented.

As mentioned above, this PR is purely "cosmetic" and does not change the functionality of the code in anyway - however, it does break the user interface (the naming convention for the input Pandas DataFrame has changed).

I figured you'd decide what version bump was appropriate directly in your NSC update ?

Copy link
Collaborator

@regDaniel regDaniel left a comment

Choose a reason for hiding this comment

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

Thanks for this rather tedious work @fpavogt ! I have nothing to add and will rebase this into my branch once it's merged.

@regDaniel
Copy link
Collaborator

On second thought... It's probably easier to merge my PR first as it's more localized and then rebase this one. I'll take care of it.

@fpavogt
Copy link
Member Author

fpavogt commented Apr 9, 2024

Sounds good. Should we bump the code version in this PR then ? I'd suggest to bump it to 1.1.0 ?

@regDaniel
Copy link
Collaborator

regDaniel commented Apr 9, 2024

Yes we should, but I wonder whether we'd even need a major upgrade to 2.0 as the changes are somewhat breaking. In any case, I can take care of it if you want.

@regDaniel regDaniel merged commit f73ce81 into develop Apr 10, 2024
15 checks passed
@fpavogt fpavogt deleted the develop_vof branch April 16, 2024 11:50
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.

2 participants