pavoljuhas commented 4 years ago

[x] make sure pickles are created with protocol 2 otherwise PY3-saved projects would not load in PY2

codecov[bot] commented 4 years ago

Codecov Report

Merging #42 into master will increase coverage by 0.08%. The diff coverage is 44.52%.

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
+ Coverage   57.39%   57.47%   +0.08%     
==========================================
  Files          96       95       -1     
  Lines       10994    10985       -9     
==========================================
+ Hits         6310     6314       +4     
+ Misses       4684     4671      -13

Impacted Files	Coverage Δ
src/diffpy/pdfgui/applications/pdfgui.py	`0.00% <0.00%> (ø)`
src/diffpy/pdfgui/control/pdflist.py	`29.78% <0.00%> (ø)`
src/diffpy/pdfgui/gui/bondlengthdialog.py	`0.00% <0.00%> (ø)`
src/diffpy/pdfgui/gui/debugoptions.py	`71.42% <0.00%> (ø)`
src/diffpy/pdfgui/gui/errorreportdialog.py	`21.81% <0.00%> (ø)`
src/diffpy/pdfgui/gui/errorwrapper.py	`38.46% <0.00%> (ø)`
src/diffpy/pdfgui/gui/mainframe.py	`48.76% <0.00%> (ø)`
src/diffpy/pdfgui/gui/phaseconstraintspanel.py	`48.33% <0.00%> (ø)`
src/diffpy/pdfgui/gui/phasepanelutils.py	`42.24% <0.00%> (ø)`
src/diffpy/pdfgui/control/plotter.py	`32.64% <6.25%> (-0.20%)`	:arrow_down:
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0509b0e...60c62ef. Read the comment docs.

dragonyanglong commented 4 years ago

Hi @pavoljuhas , I finished the test on 2.7, 3.5, 3.6 and 3.7, and the republic release PDFgui 1.1.2. I tested on both Linux and Mac OS. They gave the same error. I listed below.

Part I: install the support-python-3 branch pdfgui in Py27, 35, 36, and 37. Run in development mode. Create a project and save the ddp file. Try use other py version pdfgui (and the public release PDFgui 1.1.2) to open the four .ddp files. For example, use Py37 pdfgui to create a new project and save into ddp file, then use Py27, 35, 36 to open it.

If use Py27 PDFgui or the public release PDFgui 1.1.2 to open .ddp files generated by 35, 36, and 37, it shows an error message box:

Invalid or corrupted project /home/ly0/Documents/billinge/dev/test_pdfgui_py3/project_py37.ddp.

I screenshot the .ddp file generated by Py37 PDFgui here as an example.

Other cases are fine. Like Py 37 PDFgui can successfully open the .ddp files generated by Py27, 35, 36 PDFgui.

Part II: install the support-python-3 branch pdfgui in Py27, 35, 36, and 37. Open Emil’s old tutorial ddp files in these four version pdfgui, and go through the tutorial(https://gitlab.thebillingegroup.com/tutorials/1809_xpdpdfworkshop/tree/master/PDFgui-practical/PDFgui-tutorial).

Py37, Py36, Py35

Copy function. Right click one fit, phase or data, click “copy”. It gives:

Traceback (most recent call last): same in mac
File "/home/ly0/Documents/billinge/dev/test_pdfgui_py3/diffpy.pdfgui/src/diffpy/pdfgui/gui/errorwrapper.py", line 59, in _f
return func(*args, **kwargs)
File "/home/ly0/Documents/billinge/dev/test_pdfgui_py3/diffpy.pdfgui/src/diffpy/pdfgui/gui/mainframe.py", line 1707, in onCopy
self.treeCtrlMain.CopyBranch(selections[0])
File "/home/ly0/Documents/billinge/dev/test_pdfgui_py3/diffpy.pdfgui/src/diffpy/pdfgui/gui/fittree.py", line 560, in CopyBranch
cdatastring = "pdfgui_cliboard=" + cdatastring
TypeError: can only concatenate str (not "bytes") to str

Cannot load a Ni structure from file. Create new fit, add phase, open a Ni cif file (Ni.cif in the Emil’s tutorial.). (https://gitlab.thebillingegroup.com/tutorials/1809_xpdpdfworkshop/blob/master/PDFgui-practical/PDFgui-tutorial/04-Ni-Si-mixture/Ni.cif) It gives:

Traceback (most recent call last):
File "/home/ly0/Documents/billinge/dev/test_pdfgui_py3/diffpy.pdfgui/src/diffpy/pdfgui/gui/errorwrapper.py", line 59, in _f
return func(*args, **kwargs)
File "/home/ly0/Documents/billinge/dev/test_pdfgui_py3/diffpy.pdfgui/src/diffpy/pdfgui/gui/addphasepanel.py", line 163, in onOpen
insertafter=self.entryphase, filename=self.fullpath)
File "/home/ly0/Documents/billinge/dev/test_pdfgui_py3/diffpy.pdfgui/src/diffpy/pdfgui/gui/fittree.py", line 411, in AddPhase
self.control.loadStructure(pdata, filename, label, pos)
File "/home/ly0/Documents/billinge/dev/test_pdfgui_py3/diffpy.pdfgui/src/diffpy/pdfgui/control/pdfguicontrol.py", line 221, in loadStructure
struct.initial.read(filename)
File "/home/ly0/Documents/billinge/dev/test_pdfgui_py3/diffpy.pdfgui/src/diffpy/pdfgui/control/fitstructure.py", line 114, in read
p = PDFStructure.read(self, filename, format)
File "/home/ly0/Documents/billinge/dev/test_pdfgui_py3/diffpy.pdfgui/src/diffpy/pdfgui/control/pdfstructure.py", line 53, in read
p = PDFFitStructure.read(self, filename, format)
File "/home/ly0/anaconda3/envs/pdfgui_py37_test/lib/python3.7/site-packages/diffpy.structure-3.0.1-py3.7.egg/diffpy/structure/pdffitstructure.py", line 58, in read
p = Structure.read(self, filename, format)
File "/home/ly0/anaconda3/envs/pdfgui_py37_test/lib/python3.7/site-packages/diffpy.structure-3.0.1-py3.7.egg/diffpy/structure/structure.py", line 236, in read
new_structure = p.parseFile(filename)
File "/home/ly0/anaconda3/envs/pdfgui_py37_test/lib/python3.7/site-packages/diffpy.structure-3.0.1-py3.7.egg/diffpy/structure/parsers/p_auto.py", line 89, in parseFile
return self._wrapParseMethod("parseFile", filename)
File "/home/ly0/anaconda3/envs/pdfgui_py37_test/lib/python3.7/site-packages/diffpy.structure-3.0.1-py3.7.egg/diffpy/structure/parsers/p_auto.py", line 110, in _wrapParseMethod
stru = pmethod(*args, **kwargs)
File "/home/ly0/anaconda3/envs/pdfgui_py37_test/lib/python3.7/site-packages/diffpy.structure-3.0.1-py3.7.egg/diffpy/structure/parsers/structureparser.py", line 80, in parseFile
s = fp.read()
File "/home/ly0/anaconda3/envs/pdfgui_py37_test/lib/python3.7/codecs.py", line 322, in decode
(result, consumed) = self._buffer_decode(data, self.errors, final)
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xa9 in position 17: invalid start byte

Other cif files like Si.cif in the tutorial seems like working fine.

Sometimes, After create a fit tree in the panel. Right click the fit, click “insert a phase” or “insert dataset”, it shows the following. But I cannot reproduce this error everytime. It seems like usually happen when I open the Py27 version PDFgui first, then open the Py37 version. Maybe this error was just the “.pyc” files when I run in development mode left in the repo:

Traceback (most recent call last):
File "/home/ly0/Documents/billinge/dev/test_pdfgui_py3/diffpy.pdfgui/src/diffpy/pdfgui/gui/errorwrapper.py", line 59, in _f
return func(*args, **kwargs)
File "/home/ly0/Documents/billinge/dev/test_pdfgui_py3/diffpy.pdfgui/src/diffpy/pdfgui/gui/adddatapanel.py", line 95, in readConfiguration
remember = self.cP.getboolean("DATASET", "remember")
File "/home/ly0/anaconda3/envs/pdfgui_py37_test/lib/python3.7/configparser.py", line 828, in getboolean
raw=raw, vars=vars, fallback=fallback, **kwargs)
File "/home/ly0/anaconda3/envs/pdfgui_py37_test/lib/python3.7/configparser.py", line 808, in _get_conv
**kwargs)
File "/home/ly0/anaconda3/envs/pdfgui_py37_test/lib/python3.7/configparser.py", line 802, in _get
return conv(self.get(section, option, **kwargs))
File "/home/ly0/anaconda3/envs/pdfgui_py37_test/lib/python3.7/configparser.py", line 1160, in _convert_to_boolean
raise ValueError('Not a boolean: %s' % value)
ValueError: Not a boolean: ['["[\'False\']"]']

Select several cells in phase panel. If Press “shift” (or “command” in mac) and select a few cells, it works fine. But if press “control” on keyboard, click a few cells, for example one row of U33 ADPs. it shows:

Traceback (most recent call last):
File "/home/ly0/Documents/billinge/dev/test_pdfgui_py3/diffpy.pdfgui/src/diffpy/pdfgui/gui/errorwrapper.py", line 59, in _f
return func(*args, **kwargs)
File "/home/ly0/Documents/billinge/dev/test_pdfgui_py3/diffpy.pdfgui/src/diffpy/pdfgui/gui/phaseconfigurepanel.py", line 453, in onEditorShown
self._selectedCells = gridutils.getSelectedCells(self.gridAtoms)
File "/home/ly0/anaconda3/envs/pdfgui_py37_test/lib/python3.7/site-packages/diffpy.utils-3.0.0-py3.7.egg/diffpy/utils/wx/gridutils.py", line 78, in getSelectedCells
rcset.update(grid.GetSelectedCells())
TypeError: unhashable type: 'GridCellCoords'

Doping series Macro is not working (Though I guess this problem was pre-exist for a long time before)
The phase configure panel, shows “grey” on phase configuration boxes. (No constraints were applied yet)

Py27

Copy function bug also exists for Py27. Though the error message is slightly different.

Traceback (most recent call last):
File "diffpy/pdfgui/gui/errorwrapper.py", line 59, in _f
return func(*args, **kwargs)
File "diffpy/pdfgui/gui/mainframe.py", line 1707, in onCopy
self.treeCtrlMain.CopyBranch(selections[0])
File "diffpy/pdfgui/gui/fittree.py", line 561, in CopyBranch
textdata = wx.TextDataObject(cdatastring)
UnicodeDecodeError: 'utf8' codec can't decode byte 0x80 in position 16: invalid start byte

Cannot add phase/data into a fit tree. After create a fit tree in the panel. Right click the fit, click “insert a phase” or “insert dataset”, it shows the following:
```
Traceback (most recent call last):
File "diffpy/pdfgui/gui/errorwrapper.py", line 59, in _f
return func(*args, **kwargs)
File "diffpy/pdfgui/gui/addphasepanel.py", line 104, in readConfiguration
remember = self.cP.getboolean("PHASE", "remember")
File "/home/ly0/anaconda3/envs/pdfgui_py27_test/lib/python2.7/ConfigParser.py", line 369, in getboolean
if v.lower() not in self._boolean_states:
AttributeError: 'list' object has no attribute 'lower'
```
Interesting, after I got this error in Py27 pdfgui. I go back to Py35,36,37 version, it also has this same error. I am not sure whether it is because some mixing between configuration of different pdfgui version installed under development mode. This bug doesn’t exist when I test on mac OS.

After close the error dialog window shown in issue 2 above. If continue choose “load a structure from file”, select “Ni.cif” in tutorial, it gives:

Unable to read file 'Ni.cif'
Unknown or invalid structure format.
Errors per each tested structure format:
cif: Star Format error: /home/ly0/Documents/billinge/dev/test_pdfgui_py3/1809_xpdpdfworkshop/PDFgui-practical/PDFgui-tutorial/02-Ni-Neutron/Ni.cif: bad encoding (must be utf8 or ascii)
discus: 249: unit cell not defined
pdb: 1: invalid record name ''data_64989-ICSD''
pdffit: 249: file is not in PDFfit format
rawxyz: 1: invalid RAWXYZ format, expected 3 or 4 columns
xcfg: 1: first line must contain 'Number of particles ='
xyz: 1: invalid XYZ format, missing number of atoms.

dragonyanglong commented 4 years ago

I attach here the ddp files created and saved by Py37 in Linux and Mac during my testing.

Archive.zip

dragonyanglong commented 3 years ago

Hi @pavoljuhas , hope all is well. I chatted with @sbillinge about the incompatibility problem the pickel between py2 and py3. He suggests that if it is a big problem, it is okay for py3 version not to load old pdfgui ddp files. How do you think about merging this PR? Or maybe merge it into dev branch, and release it as beta version for more testing on this? Thanks.

pavoljuhas commented 3 years ago

Hi @dragonyanglong and @sbillinge, the tests above seem to say that project saved from this version installed for 3.7 does not load with 2.7. I suppose a way to go would be to support only Python 3 once this is merged. There are however other errors that should be fixed, e.g., loading of tutorial CIF files. The errors which mention configparser may be due to incompatibility or corruption of the .pdfgui.cfg configuration file. Try to remove that file from your HOME if it helps. Sorry, I have otherwise no time to follow up here.

sbillinge commented 3 years ago

Hi @dragonyanglong and @sbillinge, the tests above seem to say that project saved from this version installed for 3.7 does not load with 2.7. I suppose a way to go would be to support only Python 3 once this is merged. There are however other errors that should be fixed, e.g., loading of tutorial CIF files. The errors which mention configparser may be due to incompatibility or corruption of the .pdfgui.cfg configuration file. Try to remove that file from your HOME if it helps. Sorry, I have otherwise no time to follow up here.

Thanks @pavoljuhas! Let's drop python 2 support (it is EOL'd anyway), then just fix the other errors if possible.

dragonyanglong commented 3 years ago

Hi @pavoljuhas , hope everything goes well. I am working on fixing the issues for this PR. How would you suggest to merge this PR?

You merge this PR to pdfgui repo first, then I create PR from my own forked repo to pdfgui repo independently, or I directly push my commits to this PR (i.e. to your forked repo's branch). Or other suggestions?

Thanks so much!

pavoljuhas commented 3 years ago

Hi @dragonyanglong, there may be users who install from master so we should perhaps keep this separate so things don't break for them. I suggest we merge this to the next branch and then you can create new PR-s towards the next branch. Once the code in next is usable, you can just merge next to master and continue development in a standard way.

Are you OK with such plan? If so, I can take care of merging this PR to next.

sbillinge commented 3 years ago

all good with me.

On Wed, Nov 18, 2020 at 1:40 AM Pavol Juhas notifications@github.com wrote:

Hi @dragonyanglong https://github.com/dragonyanglong, there may be users who install from master so we should perhaps keep this separate so things don't break for them. I suggest we merge this to the next branch and then you can create new PR-s towards the next branch. Once the code in next is usable, you can just merge next to master and continue development in a standard way.

Are you OK with such plan? If so, I can take care of merging this PR to next.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/diffpy/diffpy.pdfgui/pull/42#issuecomment-729467678, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAOWUNIQOJS2CRKJ45LGLDSQNT4RANCNFSM4I25B3BA .

-- Simon Billinge Professor, Columbia University Physicist, Brookhaven National Laboratory

dragonyanglong commented 3 years ago

Hi @pavoljuhas , sounds good to me. Thanks for helping with the merging. Let's go with it then.

BTW, I was stuck at a bug about the gui for quite long time, but still couldn't find any relevant info online. In this Py3 version, the phase panel will show "Grey" even if no constrains are applied yet. So when we launch pdfgui in the development mode it would be like this:

However, if I use wxGlade software to open the corresponding phaseconfigurepanel.wxg, it looks just like normal:

Do you have any insights on this weird behavior?

pavoljuhas commented 3 years ago

@dragonyanglong - merge to the next done. Please do follow-up PRs towards the next branch.

the phase panel will show "Grey" even if no constrains are applied yet.

There is a code that greys-out and locks input fields with active constraints. Try to disable it and see if PDFgui still starts with fields greyed out.

dragonyanglong commented 3 years ago

Thanks so much, Pavol. I can now create PR to the next branch for further edits successfully.

Regarding the grey background color in parameter boxes, thanks for the hint, I found this change-background-color function at https://github.com/diffpy/diffpy.pdfgui/blob/master/src/diffpy/pdfgui/gui/phaseconfigurepanel.py#L252-L269. If we disable it, no problems anymore. So I suspect it is due to when using newer wxpython version, which changes from txtbg = self.textCtrlA.GetDefaultAttributes().colBg (py2) to txtbg = self.textCtrlA.DefaultStyle.BackgroundColour (py3).

I was wondering how did you decide to change these wxpython commands when upgrading from py2 to py3, just googling the function one by one? Or any automatic process? I currently fixed this bug by forcing textCtrl.SetBackgroundColour(txtbg) to be textCtrl.SetBackgroundColour(wx.WHITE), but this is not that smart as your original code, that's why I would like to know how you upgrade wxpython codes from py2 to py3?

sbillinge commented 3 years ago

@dragonyanglong should this PR be closed and you will make a new one into next? Please advise.

pavoljuhas commented 3 years ago

I was wondering how did you decide to change these wxpython commands when upgrading from py2 to py3, just googling the function one by one? Or any automatic process?

Hi Long, this is not quite Python 2 vs 3 issue, instead wxPython had some incompatible changes in version 4 (Phoenix). PDFgui was developed in wxPython <= 3.0. I mostly followed this guide to adjust wxPython calls - https://wxpython.org/Phoenix/docs/html/MigrationGuide.html. You may want to skim over https://github.com/diffpy/diffpy.pdfgui/pull/34 if there are some reusable changes.

I currently fixed this bug by forcing textCtrl.SetBackgroundColour(txtbg) to be textCtrl.SetBackgroundColour(wx.WHITE), but this is not that smart as your original code, that's why I would like to know how you upgrade wxpython codes from py2 to py3?

txtbg should change depending on whether the GUI field is constrained or not. I guess you need to track down why it has incorrect value for unconstrained fields.

sbillinge commented 3 years ago

5/21 - found that these commits are all merged into branch next.

Leave this for now, and close later, after we merge nextinto masterwhen everything is working on next

diffpy / diffpy.pdfgui

Support Python 3 in addition to Python 2 #42

Codecov Report