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

Update create_abd_from_h5 to include other post-processing options; update documentation #92

Merged
merged 5 commits into from
May 29, 2024
Merged

Conversation

keefemitman
Copy link
Contributor

Update the create_abd_from_h5 function to include other post-processing, i.e., time translate by the worldtube radius, scale out the total Christodoulou mass to make waveforms dimensionless, interpolate on to some coarser time array, and map to some reasonable BMS frame. Documentation in docs/tutorial_abd.rst has also been updated.

keefemitman and others added 3 commits May 22, 2024 14:16
Add description of BMS frames (i.e., scri.asymptotic_bondi_data.map_to_superrest_frame) and how to use scri.SpEC.file_io.create_abd_from_h5 to process SpECTRE CCE output.
Copy link

@nilsdeppe nilsdeppe left a comment

Choose a reason for hiding this comment

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

This is really great! Thank you so much! I haven't yet tried to run it on CCE output, but here are some suggestions from a careful reading. Please just incorporate what you agree with and ignore what you don't :)

docs/tutorial_abd.rst Outdated Show resolved Hide resolved
docs/tutorial_abd.rst Outdated Show resolved Hide resolved
docs/tutorial_abd.rst Outdated Show resolved Hide resolved
docs/tutorial_abd.rst Outdated Show resolved Hide resolved
docs/tutorial_abd.rst Outdated Show resolved Hide resolved
docs/tutorial_abd.rst Outdated Show resolved Hide resolved
t_0_superrest=1600,
padding_time=200
)
>>> h = scri.asymptotic_bondi_data.map_to_superrest_frame.MT_to_WM(2.0*abd.sigma.bar)

Choose a reason for hiding this comment

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

So this call was non-intuitive to me, even after reading the entire ABD tutorial, including what you added here. Here are a few thoughts to consider for making this easier to understand:

  1. Having a section in this ABD tutorial on how to get the strain from an ABD object. This would state that h=2\bar{\sigma} (though currently the ABD tutorial states it's without a factor of 2?), that we need to convert from the ABD representation to a WaveformModes (which my understanding is what MT_to_WM does, maybe linking to the tutorial for that?), and then also linking again to the WM tutorial section "Working with WaveformModes".
  2. Having somewhere, either here or in the "Working with WaveformModes" an explanation of how to plot the (2,2) of the strain. I expect this is something that someone will want to do as a first sanity check (that's what I was trying to do as a first sanity check) and so we should essentially spoon feed this :)
  3. Adding a code comment or a description after this code block that just points folks to where they can find the details on this last line. Again, the "Working with WaveformModes" seems like the right place, but I don't know :)

Copy link

Choose a reason for hiding this comment

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

I think 2. of Nils' suggestions is probably the most important, because yeah I agree that people will just want to see a waveform when they are first trying this out. If they want to do more complicated things they'll have to read the papers and such.

I also agree there should be links to the docs on what a WaveformModes object is and does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

@knelli2 knelli2 left a comment

Choose a reason for hiding this comment

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

I'll be linking to this exact page in the CCE tutorial

docs/tutorial_abd.rst Show resolved Hide resolved
t_0_superrest=1600,
padding_time=200
)
>>> h = scri.asymptotic_bondi_data.map_to_superrest_frame.MT_to_WM(2.0*abd.sigma.bar)
Copy link

Choose a reason for hiding this comment

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

I think 2. of Nils' suggestions is probably the most important, because yeah I agree that people will just want to see a waveform when they are first trying this out. If they want to do more complicated things they'll have to read the papers and such.

I also agree there should be links to the docs on what a WaveformModes object is and does

scri/SpEC/file_io/__init__.py Outdated Show resolved Hide resolved
scri/SpEC/file_io/__init__.py Outdated Show resolved Hide resolved
scri/SpEC/file_io/__init__.py Outdated Show resolved Hide resolved
scri/SpEC/file_io/__init__.py Outdated Show resolved Hide resolved
scri/SpEC/file_io/__init__.py Outdated Show resolved Hide resolved
scri/SpEC/file_io/__init__.py Outdated Show resolved Hide resolved
scri/SpEC/file_io/__init__.py Outdated Show resolved Hide resolved
scri/SpEC/file_io/__init__.py Outdated Show resolved Hide resolved
docs/tutorial_abd.rst Show resolved Hide resolved
scri/SpEC/file_io/__init__.py Outdated Show resolved Hide resolved
scri/asymptotic_bondi_data/__init__.py Show resolved Hide resolved
scri/asymptotic_bondi_data/__init__.py Outdated Show resolved Hide resolved
Copy link

@knelli2 knelli2 left a comment

Choose a reason for hiding this comment

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

I have no more comments. No need to wait for my review of the final changes to merge.

@moble moble merged commit fd8a30d into moble:main May 29, 2024
5 checks passed
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.

4 participants