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 new analysis node for creating histogram of data #241

Merged
merged 33 commits into from
Dec 20, 2021

Conversation

wpfff
Copy link
Contributor

@wpfff wpfff commented Dec 8, 2021

Added a new node that allows creating histograms of data.
Also promoted a few things (like axis selection widgets) to more general tools, and fixed a few bugs that i ran into while developing the new feature.

@astafan8
Copy link
Collaborator

astafan8 commented Dec 9, 2021

@wpfff will have a look by monday/tuesday, is htat ok?

@wpfff
Copy link
Contributor Author

wpfff commented Dec 9, 2021

Absolutely, thanks!

@astafan8 astafan8 changed the title Adding new analysis node Add new analysis node for creating histogram of data Dec 13, 2021
Copy link
Collaborator

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

Looks good! I will try to run this locally within a few days in an attempt to confirm that nothing broke :)

it would be great to make the GUI tests executed by CI... and i don't mean "assert press of button foo results in bar" but really at least just make sure these get executed and don't crash accidentally ))

:return: ``None``
"""
self.clear()
allDims = self.entries + list(dims)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this actually change self.entries and call addItem on elements of that? or is self.entries actually unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

self.entries contains currently only one element, None, to give the user the option to not select any item.
And that entry should be stored somewhere other than the actual item list, because it would be cleared when we set a new list of dimensions.
So there will in any case always at least be the None in the dropdown. It looks maybe a bit weird, but i wanted to have that information not in the function but rather in the constructor, and was thinking of cases where more than just the None could be useful to fall under that category.

plottr/node/histogram.py Outdated Show resolved Hide resolved
@wpfff
Copy link
Contributor Author

wpfff commented Dec 14, 2021

it would be great to make the GUI tests executed by CI... and i don't mean "assert press of button foo results in bar" but really at least just make sure these get executed and don't crash accidentally ))

Fair point -- i'll look into that. Do you have any experience with that? i never tried running tests with GUI elements.

@wpfff
Copy link
Contributor Author

wpfff commented Dec 17, 2021

@astafan8 i've added a test for the new node (incl the ui, at least a basic one).
do you think that looks ok?

Copy link
Collaborator

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

@wpfff thanks for the UI tests, they are a good start! going forward, i think, that these tests are very useful for verifying that the chain of signals and slots works as expected, not only the fact that the UI can render :)

@astafan8 astafan8 merged commit a31b3d4 into toolsforexperiments:master Dec 20, 2021
@wpfff
Copy link
Contributor Author

wpfff commented Dec 20, 2021

Thanks @astafan8 -- great idea, I agree this would be useful. I started to look into pytest-qt more, I'll probably improve some things with it in the coming weeks.

@wpfff wpfff deleted the feature/histogram-node branch January 10, 2022 13:12
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.

2 participants