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

Extend formal integral to line_interaction_type macroatom #856

Merged
merged 2 commits into from
Jul 31, 2018

Conversation

chvogl
Copy link
Contributor

@chvogl chvogl commented Jul 31, 2018

No description provided.

Copy link
Member

@wkerzendorf wkerzendorf left a comment

Choose a reason for hiding this comment

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

looks very good. I think we can remove some of the indexing fun - but otherwise great work!!

no_shells = len(model.w)

if runner.line_interaction_type == 'macroatom':
ma_int_data = macro_data.query('transition_type >=0')
Copy link
Member

Choose a reason for hiding this comment

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

never seen query before.


if runner.line_interaction_type == 'macroatom':
ma_int_data = macro_data.query('transition_type >=0')
internal_jump_mask = (macro_data.transition_type >= 0).values
Copy link
Member

Choose a reason for hiding this comment

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

why do you use both the mask and the query

ma_int_data['atomic_number'].values,
ma_int_data['ion_number'].values,
ma_int_data['source_level_number'].values]
)
Copy link
Member

Choose a reason for hiding this comment

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

that exists already in the atomic_data.macro_atom_data. it is called source_level_idx and destination_level_idx (I think the first one might not exits - but we should but it there).

columns=np.arange(no_shells), index=macro_ref.index
)
q_indices = (source_level_idx, destination_level_idx)
for shell in range(no_shells):
Copy link
Member

Choose a reason for hiding this comment

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

xrange

@wkerzendorf
Copy link
Member

This looks very good. I'm merging

@wkerzendorf
Copy link
Member

@chvogl

Copy link
Member

@wkerzendorf wkerzendorf left a comment

Choose a reason for hiding this comment

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

looks good!

@wkerzendorf wkerzendorf merged commit 29f0193 into tardis-sn:master Jul 31, 2018
@wkerzendorf
Copy link
Member

@chvogl there is a comment in atomic about destination_level_idx which should be called reference_idx. Any opinion on that?

@chvogl
Copy link
Contributor Author

chvogl commented Jul 31, 2018

@wkerzendorf In my opinion we can remove this comment.

@wkerzendorf
Copy link
Member

let's do that. it just causes additional confusion. I can make a PR. I also want to log to stdout instead of err - for ease of use in jupyter notebooks and will combine the PRs

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.

2 participants