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

Remove non-generic nexus components #112

Merged
merged 8 commits into from
Oct 2, 2024
Merged

Conversation

SimonHeybrock
Copy link
Member

These were added initially when it was not clear that we would go with a fully generic workflow, and left in since I thought they might be useful for debugging or general exploratory work. However, it now proves that they simply add friction in further code changes, so there is no benefit.

Fixes #103 by removing it.

I have checked on ESSsans that downstream packages will simply have to update a few imports, i.e., updating this will be trivial.

There are no code changes here, apart form removing forwarding and updating type-hints.

Comment on lines 194 to 201
def to_snx_selection(self, *, for_events: bool) -> snx.typing.ScippIndex:
if self.value == slice(None, None):
return ()
if isinstance(self.value, slice):
if for_events:
return {'event_time_zero': self.value}
return {'time': self.value}
return self.value
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate of _nexus_loader._to_snx_selection. Why did you change the loader to take a raw selection instead of a PulseSelection object?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because PulseSelection is now generic, so that didn't work.

Thought I removed this (I was moving it back and forth), good catch.

# include the end
bounds = sc.arange(
'chunk', info.start_time, info.end_time + chunk_length, chunk_length
def GenericNeXusWorkflow() -> sciline.Pipeline:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def GenericNeXusWorkflow() -> sciline.Pipeline:
def LoadNeXusWorkflow() -> sciline.Pipeline:

Since you removed 'generic' everywhere else.

Copy link
Member Author

Choose a reason for hiding this comment

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

"Generic" here means "cross-technique".

Copy link
Member

Choose a reason for hiding this comment

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

You didn't use that word for the other workflows in this file. And they are also cross-technique

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see the point in changing naming in this context. If you think it is really important we can sit down and have a session on overall naming conventions.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is very important. I just think that 'GenericNeXusWorkflow' sounds like it does something conceptually different than 'LoadMonitorWorkflow'. Where actually, it just loads data and is as generic as 'LoadMonitorWorkflow'.

Copy link
Member

@jl-wynen jl-wynen left a comment

Choose a reason for hiding this comment

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

Naming is up to you

@SimonHeybrock SimonHeybrock merged commit 942194a into main Oct 2, 2024
4 checks passed
@SimonHeybrock SimonHeybrock deleted the remove-non-generic-nexus branch October 2, 2024 10:20
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.

Test NeXus workflow
2 participants