-
Notifications
You must be signed in to change notification settings - Fork 10
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 gooey support #130
add gooey support #130
Conversation
from diffpy.labpdfproc.functions import CVE_METHODS, apply_corr, compute_cve | ||
from diffpy.labpdfproc.tools import known_sources, load_metadata, preprocessing_args | ||
from diffpy.utils.parsers.loaddata import loadData | ||
from diffpy.utils.scattering_objects.diffraction_objects import XQUANTITIES, Diffraction_object | ||
|
||
|
||
def define_arguments(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we need to redefine the input and zscan file arguments for gui, I think it's easier if we put all arguments in a separate function, so that later when we update the arguments we only need to update this.
"folder ./data)." | ||
), | ||
"nargs": "+", | ||
"widget": "MultiFileChooser", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we have cli we support a mixture of multiple files, folders, and wildcards. In gui I think we can only choose one of multiple file or folder for one argument. I can maybe add another argument for selecting folders?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, if that is the case in teh gui let's have them as separate inputs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks really great! Congrats. If you make an issue to update the docs/tutorial I can merge this. Or else we should put the updates to the docs on this PR and merge it together.
"folder ./data)." | ||
), | ||
"nargs": "+", | ||
"widget": "MultiFileChooser", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, if that is the case in teh gui let's have them as separate inputs
we have to find some kind of workaround to pin testing to py 3.12 until the Gooey dependency is updated. This won't be so easy because of the centralized way we are doing testing at the moment, but let's try and find a way to just test this up to 3.12. |
@sbillinge it appears that the latest conda-forge supports py310.. https://anaconda.org/conda-forge/gooey/files Also, a similar issue that remains open: |
I am running gooey on Regolith in a 3.12 env. So it is not a problem with the code per se, but the issue is how we run our tests and design our requirements. ATM I think we are pulling a file from |
I read online that we can use |
@yucongalicechen we have matrix testing configured https://github.com/diffpy/diffpy.labpdfproc/blob/main/.github/workflows/matrix-and-codecov-on-merge-to-main.yml in this repository. But, again, it's testing against 3.11, 3.12, 3.13. @sbillinge One possibility is that we could dynamically override default Python versions (3.11, 3.12, 3.13) here and just make sure we remove it later: From
to
|
Another possibility is that instead of |
Thankd @bobleesj I think either of those solutions can work. The basic idea is that we for sure will have to make some local change in the GitHub workflow and then make an issue to change it back later, but what is the minimal change that just kind of gets the job done. But the update needs to be in the local directory, not in release-packages (I forget the name exactly) it it compromises tests across all our stack. |
seems like wxpython pip install fails here in the CI. Did you test pip install using conda env with Python 3.13 before? |
It gets complicated but we could conda install wx then pip install gooey, or just only test up to 3.12 |
I just checked that Python 3.13 works on my computer |
Thanks. I will look more into this tmr! |
@yucongalicechen Let's try re-running it now. #136 is merged so that it the linux CI can build wxpython. |
Working nicely!! Thanks Bob!! |
@sbillinge I think this PR is ready to be merged. I've added issues #131 and #132 to take care of the docs and input argument. |
@yucongalicechen it just takes a bit of time (28 mins) to build wxpython here for Linux.. (uses sdist instead of whl files since PyPI provides no linux whl file) |
closes #57
It's working for both CLI arguments and GUI. Here're some screenshots.
@sbillinge ready for some feedback!