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

Refactor/bsk 321 denton flux module #322

Merged
merged 4 commits into from
May 30, 2023

Conversation

JulianHammerl
Copy link
Contributor

@JulianHammerl JulianHammerl commented May 5, 2023

Description

The particle flux units were changed from [cm^-2 s^-1 sr^-2 eV^-1] to [m^-2 s^-1 sr^-2 eV^-1]. This required changes in the PlasmaFluxMsg message and the DentonFluxModel module. The code was cleaned cleaned up along to way, whatever caught my eye.

Verification

The existing UnitTest was updated to reflect the changes and still passes.

Documentation

No .rst documentation was invalidated. The comments in the PlasmaFluxMsg message and the DentonFluxModel module files. were updated accordingly to reflect the change in units

Future work

None expected at this point.

@JulianHammerl JulianHammerl added the refactor Clean up with no new functionality label May 5, 2023
@JulianHammerl JulianHammerl self-assigned this May 5, 2023
@JulianHammerl JulianHammerl linked an issue May 5, 2023 that may be closed by this pull request
@juan-g-bonilla
Copy link
Contributor

juan-g-bonilla commented May 5, 2023

@JulianHammerl linux test errors are due to #313, so don't worry about them . Strange that all tests passed in that PR but not now. Will fix soon, sorry.

Copy link
Contributor

@schaubh schaubh left a comment

Choose a reason for hiding this comment

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

Needs to add release notes explaining the change in units in this message.

@JulianHammerl JulianHammerl force-pushed the refactor/bsk-321-denton-flux-module branch 2 times, most recently from a8eafd4 to 934f6f5 Compare May 8, 2023 14:55
Copy link
Contributor

@schaubh schaubh left a comment

Choose a reason for hiding this comment

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

should be good to go.

Copy link
Contributor

@leahkiner leahkiner 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! My only feedback is that you can remove the constructor and destructor definitions in the dentonFluxModel.cpp and simply add = default into the header file definitions if you want the code to be more minimal. But that's not necessary!

No functional changes
From [cm^-2 s^-1 sr^-2 eV^-1] to [m^-2 s^-1 sr^-2 eV^-1]
@JulianHammerl JulianHammerl force-pushed the refactor/bsk-321-denton-flux-module branch from 934f6f5 to ca11cbf Compare May 30, 2023 16:05
@JulianHammerl JulianHammerl force-pushed the refactor/bsk-321-denton-flux-module branch from ca11cbf to 3e20b24 Compare May 30, 2023 16:35
@JulianHammerl JulianHammerl merged commit 7ba826c into develop May 30, 2023
@JulianHammerl JulianHammerl deleted the refactor/bsk-321-denton-flux-module branch May 30, 2023 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Clean up with no new functionality
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Refactor Denton Flux Module
5 participants