BSPlotterProjected does not work

shyuep commented 7 years ago

System

Pymatgen version: master
Python version: 3.6.2
OS version: Mac

Summary

@hautierg @computron Basically, a simple supply of the BSPlotterProjected, especially the get_projected_plots_dots_patom_pmorb generates errors. E.g.,

ERROR: test_methods (__main__.BSPlotterProjectedTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/shyue/miniconda3/lib/python3.6/site-packages/matplotlib/units.py", line 145, in get_converter
    if not np.all(xravel.mask):
AttributeError: 'numpy.ndarray' object has no attribute 'mask'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/shyue/repos/pymatgen/pymatgen/electronic_structure/tests/test_plotter.py", line 136, in test_methods
    {'Cu': [3, 5], 'O': [1]}
  File "/Users/shyue/repos/pymatgen/pymatgen/electronic_structure/plotter.py", line 1474, in get_projected_plots_dots_patom_pmorb
    'b-', linewidth=band_linewidth)
  File "/Users/shyue/miniconda3/lib/python3.6/site-packages/matplotlib/pyplot.py", line 3317, in plot
    ret = ax.plot(*args, **kwargs)
  File "/Users/shyue/miniconda3/lib/python3.6/site-packages/matplotlib/__init__.py", line 1898, in inner
    return func(ax, *args, **kwargs)
  File "/Users/shyue/miniconda3/lib/python3.6/site-packages/matplotlib/axes/_axes.py", line 1406, in plot
    for line in self._get_lines(*args, **kwargs):
  File "/Users/shyue/miniconda3/lib/python3.6/site-packages/matplotlib/axes/_base.py", line 407, in _grab_next_args
    for seg in self._plot_args(remaining, kwargs):
  File "/Users/shyue/miniconda3/lib/python3.6/site-packages/matplotlib/axes/_base.py", line 385, in _plot_args
    x, y = self._xy_from_xy(x, y)
  File "/Users/shyue/miniconda3/lib/python3.6/site-packages/matplotlib/axes/_base.py", line 217, in _xy_from_xy
    bx = self.axes.xaxis.update_units(x)
  File "/Users/shyue/miniconda3/lib/python3.6/site-packages/matplotlib/axis.py", line 1413, in update_units
    converter = munits.registry.get_converter(data)
  File "/Users/shyue/miniconda3/lib/python3.6/site-packages/matplotlib/units.py", line 158, in get_converter
    converter = self.get_converter(next_item)
  File "/Users/shyue/miniconda3/lib/python3.6/site-packages/matplotlib/units.py", line 161, in get_converter
    if converter is None and iterable(x) and (len(x) > 0):
TypeError: object of type 'map' has no len()

Example code

See code in the corresponding test.

Suggested solution (if any)

Pls fix the plotter with unittests. Unfortunately, I do have to insist that this is accompanied by unittests this time. It has been 5 years since the last change. Because there are no unittests, this has gone unfixed.
Pls get this done soon, i.e., within the next week or so.

computron commented 7 years ago

I have nothing to do with this. I coded BSDOSPlotter only. I'm happy to support the plotter I wrote but that does not put me on the hook for fixing previous plotting codes that I had no hand in writing (and frankly never use).

Honestly pymatgen should just encourage use of BSDOSPlotter...

shyuep commented 7 years ago

@computron Ok. In that case, unless anyone has objections in the next one week or so and/or fixes BSPlotterProjected, I will just completely delete the old BS plotter.

computron commented 7 years ago

A few notes:

The BSDOSPlotter requires both a bs and dos. Before deleting the old plotter, we should change BSDOSPlotter so that the DOS is optional and you can only plot the BS if desired (or perhaps only the dos if the bs is None). I don't think this would be difficult to do.
The projections that BSDOSPlotter can plot are limited to "by element" and "by orbital" only. There is no function to project by both element and orbital or by anything more complicated (e.g. by site). If this is there in BSPlotterProjected, then you would lose functionality by having BSDOSPlotter only unless the latter was modified to include more complex projections.

shyuep commented 7 years ago

I will let you figure out (i). For (ii), as far as I can tell, BSPlotterProjected does allow you to specify sites, but given that it does not work right now, it is not a relevant consideration. I would suggest making BSDOSPlotter more flexible to allow those kind of projections, which I don't think is difficult at all.

The current BSPlotterProjected takes in a complicated list of orbitals and site numbers, which does not even seem to be zero-index based. Unless someone takes the effort to fix this, I am inclined to delete it because it is unusable at the moment anyway.

computron commented 7 years ago

Ok I don't see any problems with finishing (i) this week. For the site projections, I'll code it when I need it but would certainly be happy to see it happen w/o my involvement.

hautierg commented 7 years ago

I’ll look at this.

On the long-term, merging all in BSDOSPlotter could be a good move but we have to make sure we do not loose functionality.

Geoffroy

On 21 Aug 2017, at 02:16, Anubhav Jain notifications@github.com wrote:

Ok I don't see any problems with finishing (i) this week. For the site projections, I'll code it when I need it but would certainly be happy to see it happen w/o my involvement.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/materialsproject/pymatgen/issues/795#issuecomment-323621151, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3l935i_b5CKsVHOcIUTBxZNXMgPMt8ks5saMxBgaJpZM4O8rnG.

anhhv commented 7 years ago

Dear all,

I coded the "get_projected_plots_dots_patom_pmorb" method. I cloned the newest pymatgen and checked it. From my first look it works. I think the point might be at the incorrect site-numbers of "Cu" and "O" you supported ({'Cu': [3, 5], 'O': [1]}). These numbers should be complied with structure computed. Actually, if you give wrong site numbers, you will get error and notification (_number_of_subfigures method deals with this). When I check on my own computer, for instance, I put {'Cu': [3, 5], 'O': [1]} and get

ValueError: You put wrong site numbers in 'dictpa[O]': 1.

I do not know why you did not see something like that. Could you start with change in line 136 of "~/pymatgen/electronic_structure/tests/test_plotter.py", for example, {'Cu': [2], 'O': [3,4]}, and recheck again.

Best regards, Viet-Anh HA.

shyuep commented 7 years ago

I am not sure what you are referring to. I tried

plotter.get_projected_plots_dots_patom_pmorb(
            {'Cu': ['dxy', 's', 'px'], 'O': ['px', 'py', 'pz']},
            {'Cu': [2], 'O': [3, 4]}
        )

and I get the wrong site numbers error.

The original version gave me the error I wrote above.

Note that I am running Python 3.6, not 2.7. What I noticed is that you used some calls to map. In python 3.6, map returns a generator, not a list. A generator has no len.

shyuep commented 7 years ago

I just pushed something which I think will fix it.

shyuep commented 7 years ago

I should note that the structure in the BS object is O O Cu Cu Cu Cu species order. I am not sure why you think Cu is 2 and O is 3 and 4. In any case, does it work with zero indexing or is it 1 index? I tried 0 and it didn't work.

anhhv commented 7 years ago

@shyuep I checked with AlCuO2 structure, thus, the orders of O and Cu are different. The index should be >=1. I am running with python 2.7. I will continue to check.

anhhv commented 7 years ago

@shyuep The errors are at map call as you said. I used this twice and now I correct by putting "map" as an argument of "list" function, list(map(...)). For sure, I checked and it works for both python 2.x and python 3.x. I pushed my changes to my pymatgen version on github and asked for merging.

materialsproject / pymatgen