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

Fixed the array types when interpolate_shells > 0. Now returns the p… #1464

Merged
merged 1 commit into from
Feb 19, 2021

Conversation

Rodot-
Copy link
Contributor

@Rodot- Rodot- commented Feb 19, 2021

When interpolate_shells > 0, the formal integral broke with an attribute error claiming that numpy arrays had no attribute "values"
Issue #1463

I've corrected the return types of one of the functions so that they are no longer numpy arrays.

Description

When interpolate_shells > 0, interpolate_integrator_quantities would change the type of electron_densities_integ, tau_sobolev_integ, and Jredlu from pandas dataframes or series to numpy arrays due to the fact that scipy.interp1d always returns a numpy array. This was problematic because the formal_integral Cython wrapper access the .values attributes of these arrays assuming they were pandas objects. I've corrected this so now these arrays are set to have the correct type.

Motivation and Context

Now interpolate_shells can have values greater than 0 without failing.

How Has This Been Tested?

  • Testing pipeline
  • Reference Data Comparison following these instructions
  • Other (please describe)

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • None of the above (please describe)

Checklist:

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have built the documentation on my fork following these instructions
  • I have assigned and requested two reviewers for this pull request

…oper pandas series that are required for the cython formal integral wrapper
@Rodot- Rodot- requested review from wkerzendorf and chvogl and removed request for wkerzendorf February 19, 2021 17:48
@codecov
Copy link

codecov bot commented Feb 19, 2021

Codecov Report

Merging #1464 (a5f8151) into master (927c606) will decrease coverage by 1.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1464      +/-   ##
==========================================
- Coverage   71.13%   70.00%   -1.14%     
==========================================
  Files          67       67              
  Lines        5523     5314     -209     
==========================================
- Hits         3929     3720     -209     
  Misses       1594     1594              
Impacted Files Coverage Δ
...dis/tardis/montecarlo/montecarlo_numba/r_packet.py 23.15% <0.00%> (-7.83%) ⬇️
.../tardis/montecarlo/montecarlo_numba/interaction.py 24.32% <0.00%> (-5.68%) ⬇️
.../montecarlo/montecarlo_numba/single_packet_loop.py 24.56% <0.00%> (-3.78%) ⬇️
...rdis/tardis/montecarlo/montecarlo_numba/vpacket.py 15.00% <0.00%> (-2.48%) ⬇️
tardis/tardis/plasma/properties/util/macro_atom.py 30.00% <0.00%> (-2.26%) ⬇️
tardis/tardis/widgets/sdec_plot.py 14.35% <0.00%> (-1.94%) ⬇️
...rdis/montecarlo/montecarlo_numba/tests/conftest.py 91.11% <0.00%> (-1.88%) ⬇️
tardis/tardis/plasma/properties/nlte.py 39.21% <0.00%> (-1.74%) ⬇️
...s/tardis/montecarlo/montecarlo_numba/macro_atom.py 43.75% <0.00%> (-1.71%) ⬇️
tardis/tardis/montecarlo/spectrum.py 70.00% <0.00%> (-1.70%) ⬇️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 927c606...d0c371e. Read the comment docs.

@Rodot-
Copy link
Contributor Author

Rodot- commented Feb 19, 2021

Should also be of note: This bug really demonstrates the need for formal integral tests

Copy link
Contributor

@chvogl chvogl 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 fixing my blunder ;)

@chvogl
Copy link
Contributor

chvogl commented Feb 19, 2021

I also agree that we should include a test with interpolate_shells > 0, which is the setting people should be using.

@chvogl chvogl merged commit d92464c into master Feb 19, 2021
@chvogl
Copy link
Contributor

chvogl commented Feb 19, 2021

I took the liberty to merge this without a second review. The changes are minimal and fix an urgent problem.

atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
Now returns the proper pandas series that are required for the cython formal integral wrapper (tardis-sn#1464)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants