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

Opacities Refactor #2609

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

wkerzendorf
Copy link
Member

@wkerzendorf wkerzendorf commented May 6, 2024

linelist with 29k lines and 10 shells is slower by a factor of 3 in the new implementation (but I think this is shrinking with more lines; used to be a factor >5 for 247 lines).
Function took 0.001744791865348816 seconds to run
Function took 0.0059948337730020285 seconds to run

@wkerzendorf wkerzendorf changed the title add isotopic_number_density Opacities Refactor May 6, 2024
@wkerzendorf wkerzendorf force-pushed the restructure/plasma_opacities branch from 2fbdbc1 to e804942 Compare May 6, 2024 20:38
…ace.py, transport_montecarlo_numba_interface.py, conftest.py, formal_integral.py, base.py, and macro_atom.py
@andrewfullard andrewfullard self-requested a review May 7, 2024 16:06
@andrewfullard andrewfullard added restructure opacity Related to opacity extensions in TARDIS labels May 7, 2024
transition_type = macro_atom_data.transition_type.values
lines_idx = macro_atom_data.lines_idx.values
tpos = macro_atom_data.transition_probability.values
util.calculate_transition_probabilities(
Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't like having two functions with identical names here, especially if one is in util of all places. But I'm not sure if this is your change or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

still draft I think

)
return transition_probabilities

def _normalize_transition_probabilities(self, transition_probabilities):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is better duplication since it is underscored. But why is it in util?

self.macro_block_references = macro_block_references
self.transition_type = transition_type

# Destination level is not needed and/or generated for downbranch
Copy link
Contributor

Choose a reason for hiding this comment

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

Good reason to split this up! Aside from all the other reasons.

time_explosion,
stimulated_emission_factor,
):
return calculate_sobolev_line_opacity(
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, this may as well be a method, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

ultimately this will be restructured completely in the future but for now I'm just carving parts for the plasma

Copy link
Member Author

Choose a reason for hiding this comment

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

@jvshields requested that I put this back into its original form and that this PR is only a restructure.

@Knights-Templars
Copy link
Member

docstrings?

@Knights-Templars Knights-Templars marked this pull request as draft May 20, 2024 14:21
@jvshields
Copy link
Contributor

Would be nice for the refactor pr to be separate from the functional code change pr.

@tardis-bot
Copy link
Contributor

*beep* *bop*

Hi, human.

The docs workflow has failed

Click here to see the build log.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-docs opacity Related to opacity extensions in TARDIS restructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants