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

Add support for binning data to the Scan component #124

Merged
merged 2 commits into from
Feb 21, 2024
Merged

Conversation

JamesWrigley
Copy link
Member

This is usually the entire point of a scan so everyone will end up doing this anyway. I also took the liberty of cleaning up the changelog a bit in 9c39677.

One thing I wasn't sure about: some people prefer the standard error instead of the standard deviation. Should we use that by default? Or make it an option?

@JamesWrigley JamesWrigley self-assigned this Feb 20, 2024
@@ -163,6 +162,83 @@ def plot(self, ax=None):

return ax

def bin(self, data):
Copy link
Collaborator

@philsmt philsmt Feb 20, 2024

Choose a reason for hiding this comment

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

I would prefer a more expressive name here, as it initially confused me what is actually binned here to what. On top of that, I'm not a fan of collisions with builtins.

As I understand, this method can take arbitrary train-resolved data and averages it for each bin this scan represents. Binning in itself doesn't mean averaging to me, it just means combining results based on some metric. What do you think of explicitly calling it .step_averages() .average_by_steps() or something similar?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, renamed it to step_average() in dd35a73 (average_by_steps() is a bit long methinks).

"motor": self.name
})

def plot_bin(self, data, title=None, xlabel=None, ylabel=None,
Copy link
Collaborator

@philsmt philsmt Feb 20, 2024

Choose a reason for hiding this comment

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

see naming above, this could correspondingly be called something like .plot_step_averages().

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed to plot_step_average() in dd35a73.

@JamesWrigley
Copy link
Member Author

As discussed this morning, I also added support in dd35a73 for specifying the standard error to be used instead of the standard deviation:
image

Copy link
Collaborator

@philsmt philsmt left a comment

Choose a reason for hiding this comment

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

No comments on the actual implementation, but I would prefer some less ambiguous naming. Really cool functionality!

I'm already wondering what other reduction may be useful next to average. Maybe that could also be a naming direction...

@JamesWrigley
Copy link
Member Author

How about bin_steps() or reduce_steps()? That would leave the door open for different reductions in the future.

@philsmt
Copy link
Collaborator

philsmt commented Feb 20, 2024

bin_steps() or reduce_steps()

Somehow these suggest something is done to steps, rather than the steps being used to sort/bin/reduce/average something else. But this may simply be due to English being a second language.

Maybe Thomas has a good idea when he's back. But I'd also be fine to use the existing names for average here, and later implement it as a specific case of a more general function.

@JamesWrigley
Copy link
Member Author

Yeah, I'm using it more loosely in a 'bin the data that was collected during these steps' way. But I don't feel very strongly about it, how about bin_by_steps()?

@philsmt
Copy link
Collaborator

philsmt commented Feb 20, 2024

how about bin_by_steps()?

I didn't dare to suggest that, but it strongly communicates the meaning to me 🙈

@JamesWrigley
Copy link
Member Author

Alrighty, renamed to bin_by_steps() in 8a6a5dc.

@JamesWrigley
Copy link
Member Author

Ok one more naming bikeshed 🙃 Is it confusing that the label on the plots is Error: ...? Like do you think people will confuse that with an actual error from the code? If so then we could call it 'uncertainty' instead.

@philsmt
Copy link
Collaborator

philsmt commented Feb 20, 2024

Ah yes, error is indeed a loaded term. I spoke to Danilo, and he strongly preferred the term "Statistical uncertainty" instead. In the docs, we should explicitly mention it's the 68% confidence interval (also called 1-sigma region).

@JamesWrigley
Copy link
Member Author

Roger that, renamed everything to 'uncertainty' in f0fc09f.

@JamesWrigley JamesWrigley added the enhancement New feature or request label Feb 20, 2024
This is usually the entire point of a scan so everyone will end up doing this
anyway.
- Uncommented installation admonition
- Fixed formatting
- Added links
@JamesWrigley
Copy link
Member Author

(I will sneakily take your earlier LGTM 😏 )

@JamesWrigley JamesWrigley merged commit a36ada4 into master Feb 21, 2024
4 checks passed
@JamesWrigley JamesWrigley deleted the scan-bin branch February 21, 2024 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants