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

remove numexpr #1821

Merged
merged 3 commits into from
Oct 27, 2021
Merged

remove numexpr #1821

merged 3 commits into from
Oct 27, 2021

Conversation

wkerzendorf
Copy link
Member

Many years ago - I used numexpr to speed up calculations - I don't think this is needed anymore.

stimulated emission factor
@codecov
Copy link

codecov bot commented Oct 26, 2021

Codecov Report

Merging #1821 (4fa8e11) into master (e4637d6) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 4fa8e11 differs from pull request most recent head 26a4c37. Consider uploading reports for the commit 26a4c37 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1821      +/-   ##
==========================================
+ Coverage   58.21%   58.22%   +0.01%     
==========================================
  Files          66       66              
  Lines        6729     6722       -7     
==========================================
- Hits         3917     3914       -3     
+ Misses       2812     2808       -4     
Impacted Files Coverage Δ
tardis/tardis/plasma/properties/ion_population.py 85.64% <0.00%> (-0.08%) ⬇️
...s/tardis/plasma/properties/radiative_properties.py 79.02% <0.00%> (+1.83%) ⬆️

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 e4637d6...26a4c37. Read the comment docs.

@tardis-bot
Copy link
Contributor

Before a pull request is accepted, it must meet the following criteria:

  • Is the necessary information provided?
  • Is this a duplicate PR?
    • If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
    • If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
  • Does it pass existing tests and are new tests provided if required?
    • The test coverage should not decrease, and for new features should be close to 100%.
  • Is the code tidy?
    • No unnecessary print lines or code comments.

@Rodot-
Copy link
Contributor

Rodot- commented Oct 26, 2021

grepping for ne.evaluate turns up these locations:

tardis/plasma/properties/radiative_properties.py:93:        stimulated_emission_factor = ne.evaluate(
tardis/plasma/properties/radiative_properties.py:354:            transition_probabilites[start_id:end_id] *= 1 / ne.evaluate(
tardis/util/base.py:308:    intensity = ne.evaluate(

and for the imports

tardis/plasma/properties/ion_population.py:7:import numexpr as ne
tardis/plasma/properties/radiative_properties.py:5:import numexpr as ne
tardis/util/base.py:6:import numexpr as ne

@@ -346,14 +343,6 @@ def _normalize_transition_probabilities(self, transition_probabilities):
transition_probabilities, self.block_references
)

def _new_normalize_transition_probabilities(self, transition_probabilites):
Copy link
Member Author

Choose a reason for hiding this comment

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

this never was used anywhere - if tests pass then we should just remove that.

@epassaro
Copy link
Member

Also try removing the dependency from the environment file @wkerzendorf

@wkerzendorf
Copy link
Member Author

Also try removing the dependency from the environment file @wkerzendorf

there are still some other numexpr hiding out in TARDIS 😄

@wkerzendorf wkerzendorf merged commit 33772e9 into tardis-sn:master Oct 27, 2021
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.

5 participants