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

Fix the definition of hydrogenic and helium-like ions #252

Merged
merged 3 commits into from
Jan 12, 2024
Merged

Fix the definition of hydrogenic and helium-like ions #252

merged 3 commits into from
Jan 12, 2024

Conversation

jwreep
Copy link
Collaborator

@jwreep jwreep commented Jan 12, 2024

Fixes #251

Please review.

I don't think that this line

B = 1 if self.hydrogenic else 2

needs to be changed since the function it's embedded in is only called for heavier elements.

@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (282b819) 90.56% compared to head (180fdc6) 90.56%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #252   +/-   ##
=======================================
  Coverage   90.56%   90.56%           
=======================================
  Files          36       36           
  Lines        2670     2670           
=======================================
  Hits         2418     2418           
  Misses        252      252           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fiasco/ions.py Outdated
Comment on lines 945 to 946
if (self.hydrogenic and self.atomic_number >= 6) or
(self.helium_like and self.atomic_number >= 10):
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if (self.hydrogenic and self.atomic_number >= 6) or
(self.helium_like and self.atomic_number >= 10):
if ((self.hydrogenic and self.atomic_number >= 6) or
(self.helium_like and self.atomic_number >= 10)):

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, just noticed the tests failed. Fixing it now...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Too much C++ in my brain.

@jwreep
Copy link
Collaborator Author

jwreep commented Jan 12, 2024

I'm not sure I understand that last check that's failing. Any idea what the issue is?

@wtbarnes
Copy link
Owner

The pre-commit job runs several style checks on the changes to ensure consistent code style across the package. I can fix that easily. You can also install the pre-commit hooks locally using these instructions: https://docs.sunpy.org/en/latest/dev_guide/contents/code_standards.html#formatting and it will fix the files for you.

@jwreep
Copy link
Collaborator Author

jwreep commented Jan 12, 2024

Thank you! I didn't know about that. I'll make sure to use it going forward.

@wtbarnes
Copy link
Owner

@pryoung also pointed out to me that the Fontes and Dere papers note that the fits to the Fontes cross-sections should be accurate for $Z\ge4$, but then there's an additional note that Li III and B V are better fit with the BT scaled cross-section data and that the Be IV cross-sections are fit using the B V cross-sections appropriately scaled by the ionization potential. Thus, the $Z\ge6$ requirement is presumably to ensure those three ions are not fit using Fontes and instead use the .diparams values.

Interestingly, .diparams values are available for C VI, despite the fact that that cross-section is fit using Fontes. A plot of these two cross-sections is shown below,

image

@wtbarnes wtbarnes merged commit 2d8e308 into wtbarnes:main Jan 12, 2024
16 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/fiasco that referenced this pull request Jan 12, 2024
wtbarnes pushed a commit that referenced this pull request Jan 12, 2024
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.

Hydrogen- and helium-like atoms only for Z > 6 and 10?
3 participants