Skip to content

Rework hextof tutorial notebook - #274

Merged
rettigl merged 16 commits into
mainfrom
notebook_rework
Mar 22, 2024
Merged

Rework hextof tutorial notebook#274
rettigl merged 16 commits into
mainfrom
notebook_rework

Conversation

@steinnymir

@steinnymir steinnymir commented Nov 16, 2023

Copy link
Copy Markdown
Member

Added explaination of all steps, and full workflow currently available for HEXTOF@FLASH.

This is a simple PR, which changes nothing in fucntionality. I'd love to merge this quickly.

Closes #251

@steinnymir steinnymir added this to the prep FLASH BeamTime milestone Nov 16, 2023
@coveralls

coveralls commented Nov 16, 2023

Copy link
Copy Markdown
Collaborator

Pull Request Test Coverage Report for Build 8266175784

Details

  • 28 of 28 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 91.57%

Totals Coverage Status
Change from base Build 8230697770: 0.08%
Covered Lines: 6007
Relevant Lines: 6560

💛 - Coveralls

@rettigl

rettigl commented Nov 16, 2023

Copy link
Copy Markdown
Member

It would make sense to solve the data download questions here as well, but if time is an issue, we can also postpone this.

@steinnymir

Copy link
Copy Markdown
Member Author

Upgrading this for use in Maxwell is now more urgent than the data problem. I'd like to merge this as is

@rettigl rettigl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like the basic instructive rewrite, so general aproval. Some things, however, do not work for me, or could be improved, see comments.

Comment thread tutorial/4_hextof_workflow.ipynb Outdated
Comment thread tutorial/4_hextof_workflow.ipynb
Comment thread tutorial/4_hextof_workflow.ipynb Outdated
Comment thread tutorial/4_hextof_workflow.ipynb
Comment thread tutorial/4_hextof_workflow.ipynb Outdated
Comment thread tutorial/4_hextof_workflow.ipynb
@rettigl

rettigl commented Nov 17, 2023

Copy link
Copy Markdown
Member

Another thing to add would be the histogram normalization, maybe.

@steinnymir

Copy link
Copy Markdown
Member Author

Another thing to add would be the histogram normalization, maybe.

Absolutely!

@zain-sohail zain-sohail left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Very thoroughly explained. I don't think a new user would have any issues getting into this workflow.

Comment thread tutorial/4_hextof_workflow.ipynb Outdated
Comment thread tutorial/4_hextof_workflow.ipynb
Comment thread tutorial/sed_config.yaml Outdated
Comment thread sed/config/flash_example_config.yaml Outdated
Comment thread sed/config/flash_example_config.yaml
Comment thread sed/config/flash_example_config.yaml Outdated
@zain-sohail

Copy link
Copy Markdown
Member

Deployment stopped because only main branch is allowed to deploy but I overrid it for checking. I think this is good to merge.

@rettigl

rettigl commented Mar 8, 2024

Copy link
Copy Markdown
Member

Yes, it's clear that the deploy did not work. I downloaded the generated artefact and checked locally that it was fine.
However, before merging there are still several issues and todos to fix in the documentation.
@steinnymir @zain-sohail please feel free to finalize

@rettigl

rettigl commented Mar 8, 2024

Copy link
Copy Markdown
Member

Another thing to add would be the histogram normalization, maybe.

Absolutely!

added

@rettigl
rettigl force-pushed the notebook_rework branch 2 times, most recently from 74554e2 to 4f72234 Compare March 13, 2024 14:15
@rettigl
rettigl merged commit 3aa1e78 into main Mar 22, 2024
@rettigl
rettigl deleted the notebook_rework branch March 22, 2024 08:32
@rettigl
rettigl restored the notebook_rework branch March 22, 2024 09:46
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.

Externally accessing flash public data (for docs)

4 participants