-
Notifications
You must be signed in to change notification settings - Fork 59
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
[ENH] 4+ Angle Polarisation widget and Hyper Spectra vector plot #765
Conversation
e8da9dc
to
6d44774
Compare
Ignore them. Silencing them is not appropriate for multithreaded applications (those impose global states). Numpy 2 to the rescue, some day. |
If I build the docs and install as a pip package, docs show up as expected. Thanks! |
6d44774
to
a7932c9
Compare
To me, the addition to HyperSpectra seems good. I asked some questions where I was unsure, but none are blockers. What I'd like is to have it refactored in a @callumgassner, please decide if I do this or you. I saw that most of the code concerning vectors is functionally well separated so this is mostly about moving methods and changing Also, even if the interface shows up in the parent (control area) it can be totally defined in a Mixin - see how I moved the axis and color selectors in #777. |
Hi Marko, I am not sure if my last commit is what you meant by your last comment, but I was curious to give it a shot myself. I think the readability is much better like this, so maybe I am not to far off? |
@callumgassner, this is exactly what I had in mind, thanks! The only thing I would add would be to move them from the BasicImagePlot into the next class, because BasicImagePlot is (not so smartly :) ) used in another add-on, Orange-SNOM, and needs to work without vector plots. The tests also fail not but should be trivial to fix. |
@callumgassner, what do you think of the proposed HyperSpectra UI reorganization? |
I did what I suggested and removed (almost) everything vector-plot-related from the |
You have a sharp eye for issues. You saw the following bug: biolab/orange3/issues/6721. So the solution is to use "exclude_attributes=True" and "exclude_classes=True" for settings that can only be metas (because the corresponding domain model is defined as such), such as was done in #707. EDIT: I fixed this. |
Why not make a new box for entering a number with a sensible default? I have a feeling I do not understand this, please explain what you meant. Thanks! |
- Code cleanup - Faster data preparation - Improved handling of inputs with different features
Also made vector plot controls more compact.
4018ed3
to
d4c6827
Compare
Let's not worry about this now. |
@callumgassner, I rebased to current master add added some slight changes (see last commits). I think this is almost good to go. We only need:
Note that in the refactor in HyperSpectra all vector settings were moved into ImagePlot. Thus, old workflows will loose HyperSpectra settings and they should be recreated (probably easier) or a migration should be written (harder, I would not do it unless you really need it). |
Thanks for the help, Marko!
I think this is better, important controls are more easily accessible now.
Basically, we would need a new field for each wavenumber value, so something like a linedit box next to each unique value in the "features" column. I briefly played around with this today using a QTableView widget but I wasn't very happy with how it was looking: |
Ah, now I get it. If it is per-wavenumber, then, well, what you prototyped makes sense in terms that you have to show both to the user. But then you have to combine settings with selections... I would do it in similarly as in the Discretize widget. Check it out! |
@callumgassner, I added a commit than changes Polar layout a bit. I removed boxes in boxes (which we try not to use), slightly reworded labels, and moved citations. Feel free to remove/edit the commit - I do not have a strong preference. |
I quite liked this idea, thanks!! I've used it to add the unique values per feature. |
I did not test your new feature but I trust you did. So, do we merge? |
I am happy for it to be merged :) |
This is a continuation of #615
The majority of changes since then have been bug fixes. Although there have been some changes to the widget's themselves:
4+ Angle Polarisation:
Hyperspectra:
These changes have all been implemented based on feedback from the previous pr and initial users.
I believe many of the issues raised in the original pr have been addressed to some extent. However, I am aware of a few possible issues the persist:
Hyperspectra:
4+ Angle Polarisation:
I feel that there might still some issues with ContextSettings. Under some conditions on new installs/widget setting resets the settings persist for incompatible datasets, and so errors are raised. I have been dealing with them as they appear and I'm not aware of any specific ones at the moment, but if you come across any please let me know.
There is still a possible memory leak (small). Memory usage creeps up by small increments for repeated (complete and interrupted) runs of the widget. I am not sure if this is specific to this widget as I've seen similar behaviour with e.g. peak fit.
For me the documentation of this widget does not show
It would be better to enable users to enter the angle of the TDM tilt per vibrational mode. As was pointed out in the previous pr, this would be unique to each one (if the specific angle is known, often it is approximated as either parallel (0 degrees) or perpendicular (90 degrees)). But I can not think of any elegant way to implement this - more than happy to take suggestions on this.
I will need to also update the documentation for this widget prior to it being merged.
I just thought it would be better to get the ball rolling for now (and see what possibly gets purged - I acknowledge that some of these additions are niche and add significant complexity to the code, so I understand if some things do not get merged in the final version :) ).
Final note - Based on the combine_visimg function in owpolar, I have a very similar function for the multifile widget for stacking visible images from multiple files. If you are interested I can create a new pr or add the changes to this one (there was brief mention of this before activity on the other pr fizzled out)