-
Notifications
You must be signed in to change notification settings - Fork 3
skpkg: move over precommit files and create pre-commit auto fixes #7
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
Conversation
|
@sbillinge ready for review. Here is the work that still needs to be done: |
|
This is great! Just checking, we worked hard to get xPDFsuite to run so that we could test all changes we make. Did you do the last step of that work and come up with the test that we will run to make sure we didn't break anything? What I have in mind is that there is something like a set of data and maybe a saved project file and to test it we run xpdfsuite and load the project file and run a few things to make sure it hasn't broken. This will be a bit of a pain to do on every commit, but at least to do it before anything gets merged for example. It is probably not worth it for these codespell and black changes, but at least if we have a plan in our head of what the workflow is going to be as we move forward on this. Of course, extensive unit tests make this much easier, but we don't really have those yet...... tl;dr, let's have an idea in our heads about what our testing protocol is going to be as we move ahead making all these py2-py3 changes and skpkg level 5 migrations. Not too much, but enough..... I would like you to decide what you think is best here. |
|
@sbillinge I've downloaded I did have some points of concern though. When first opening the GUI, some parameters in the P.S. I am not able to use the PDFgui and the Distance Calculator features. For PDFgui, the icon in the toolbox on the top is grayed out. When I try pressing "Start PDFgui" in the dropdown, nothing happens. For the distance calculator, the icon is also grayed out in the toolbox. Pressing "Distance Calculator" in the dropdown does open another GUI window, but then the "browse" feature is not allowing me to select a structure file (I had downloaded For For Do these look okay as well? If so, I'll keep these for my future testing! |
|
@sbillinge just a gentle ping here... I was wondering if the images I sent previously look alright? Is clicking through the GUI as I make changes to the repo to verify that these graphs look the same an ample test to make sure that the code is still working properly? And of course, there's also the weird bug that I found. I just wanted to confirm that it isn't an intentional feature? |
|
sorry for the delay, I saw this, but was not able to respond. The images look good. There are some things i would like to fix, for example the first mage where there is a diagonal line from 0 to 1 in F(Q), g(r) etc.. I have wanted to fix this for a long time. But as a way of testing that the code is working, this is good. The problem that the default Qmaxinst is 0 is a bug. This doesn't happen in the old version. So I guess there is soemthing we would like to look at there to get it to work. It should set, by default, to the maximum Q-value of the data. The Qmax being set to the Qmaxinst is correct. This is a feature, not a bug, because we never want Qmax to excexed Qmaxinst. Some of the other behaviors, I am not sure whether they are in the old version or not, but we should decide what we want, for example, the reset not working when a plot is open. It is good that you found these behaviors. Let's capture them on issues and we can check them on the next released version and make them behave how we want. For PDFgui, I think it greys out if PDFgui is not installed. I think it might be a bug that it is looking for some old version of PDFgui, so we will have to look at that. For the other feature, distanceprinter, I am not sure how it works either. It may compute distances from a selected cif file in the fit-tree, and then ask for a file to write the result out to. Did you try that? In that case, it would logically grey out if a cif was not pre-selected. Could you check that? I think the pearson correlations look ok, though it doesn't make too much sense to allow them to be computed on different grids, so later we can look at that. |
|
Thanks so much for the kind words! It means a lot to me to hear this. Ok, it all makes sense. Please make an issue for the things that work but are greyed out. That sounds like a bug I have an old version of the program so let me test the distanceprinter and see what the behavior is in the old program and I will report back on that. Great work! I would obviously be delighted if you wanted to stay engaged and kept contributing at whatever level you are able. I am happy to keep you in the loop and share what our priorities are, but you could contribute with PRs on any issues you find in our diffpy and regolith stacks... |







No description provided.