Skip to content

Conversation

@YooSunYoung
Copy link
Member

@YooSunYoung YooSunYoung commented Nov 5, 2024

Related to #20, #26 and Related to one of milestone for p6

The notebook is for P6 milestone report so it is expected to be refactored and integrated into a module as a reusable workflow.

#56 should be merged first.

@YooSunYoung YooSunYoung changed the base branch from main to test-file November 8, 2024 13:09
Base automatically changed from test-file to main November 8, 2024 14:13
@YooSunYoung YooSunYoung marked this pull request as ready for review November 26, 2024 15:52
@YooSunYoung YooSunYoung changed the base branch from main to update-deps November 26, 2024 15:58
@YooSunYoung YooSunYoung requested a review from nvaytet November 26, 2024 16:03
Base automatically changed from update-deps to main November 27, 2024 09:15
" .values\n",
" )\n",
" return sc.vectors(dims=['event'], values=var, unit='m')\n"
]
Copy link
Member

Choose a reason for hiding this comment

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

I feel that most of the code at the start of this notebook should be part of a submodule in the package, and not in the notebook?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, as Søren wanted everything before December,
I thought we don't have time to make them as a module, write test and documentation.

Also, McStas format is not stable at the moment so I didn't want to start working on a mcstas IO module...

Copy link
Member

Choose a reason for hiding this comment

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

ok

" return RectInfo(x_range.min(), x_range.max(), y_range.min(), y_range.max())\n",
"\n",
"\n",
"class MergedRectanglesTool(ToggleTool):\n",
Copy link
Member

Choose a reason for hiding this comment

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

I also think this code should go in a submodule, so we can just simply import the tool in the notebook?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. Then I'll do that in plopp maybe...?

@nvaytet nvaytet merged commit a81779c into main Dec 13, 2024
4 checks passed
@nvaytet nvaytet deleted the odin-simulation branch December 13, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants