plot bands with only one kpoint #3798

unkcpz · 2020-02-24T13:29:56Z

Fix #2462 : Support plotting BandsData instances with a single kpoint.

ltalirz

thanks @unkcpz for looking into this issue.

Would it be possible to add a test for this to make sure this issue does not come back?

E.g. here:

aiida-core/tests/cmdline/commands/test_data.py

Line 329 in e4c7fb5

def test_bandsexport(self):

aiida/orm/nodes/data/array/bands.py

unkcpz · 2020-02-25T01:21:29Z

Sure I will add test. Now, it serves as an attempt to address the problem, I'll perfect solution for mpl plot and gnuplot both.
Thanks for review @ltalirz .

tests/cmdline/commands/test_data.py

aiida/orm/nodes/data/array/bands.py

ltalirz

thanks @unkcpz !

I've had a quick look through and have some comments

ltalirz · 2020-02-27T10:08:55Z

aiida/orm/nodes/data/array/bands.py

@@ -825,7 +825,10 @@ def _prepare_mpl_singlefile(self, *args, **kwargs):

        s_header = matplotlib_header_template.substitute()
        s_import = matplotlib_import_data_inline_template.substitute(all_data_json=json.dumps(all_data, indent=2))
-        s_body = matplotlib_body_template.substitute()
+        if len(all_data['paths']) == 1:


It seem to me that _prepare_mpl_singlefile could simply be reusing _prepare_mpl_withjson in order to avoid code duplication. Is this correct?

Yes, these two can be combined. I'll try.

I tried, however, each of _prepare_mpl_* function is similar but slightly different. I can't think of a better way to combine them at the moment. Using a lone template string and leave all optional string as arguments seems like not a good idea.

ltalirz · 2020-02-27T10:16:19Z

aiida/orm/nodes/data/array/bands.py

@@ -1074,6 +985,115 @@ def show_mpl(self, **kwargs):
        """
        exec(*self._exportcontent(fileformat='mpl_singlefile', main_file_name='', **kwargs))  # pylint: disable=exec-used

+    def _prepare_gnuplot(self,


Is there a reason why you moved _prepare_gnuplot down in the file?
That makes it harder to spot the changes you made in review.

I just move all prepare_mpl_* functions together. _prepare_gnuplot was inserted in the middle of these _mpl* functions. I'll move it back for your review, if necessary I'll move mpl functions together at the end.

tests/cmdline/commands/test_data.py

ltalirz · 2020-04-11T17:55:13Z

@unkcpz Just merged the other PR, I guess now it makes sense to continue work on this.

codecov · 2020-04-11T18:20:20Z

Codecov Report

Merging #3798 into develop will increase coverage by 0.02%.
The diff coverage is 96.22%.

@@             Coverage Diff             @@
##           develop    #3798      +/-   ##
===========================================
+ Coverage    78.18%   78.20%   +0.02%     
===========================================
  Files          460      460              
  Lines        33971    33982      +11     
===========================================
+ Hits         26559    26575      +16     
+ Misses        7412     7407       -5

Flag	Coverage Δ
#django	`70.23% <96.22%> (+0.02%)`	⬆️
#sqlalchemy	`71.07% <96.22%> (+0.02%)`	⬆️

Impacted Files	Coverage Δ
aiida/orm/nodes/data/array/bands.py	`76.21% <96.22%> (+1.40%)`	⬆️

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 ff9e8a2...68621fc. Read the comment docs.

unkcpz · 2020-04-12T01:26:54Z

@ltalirz Seems working now:smile: After your review, I suggest to move all _prepare_mpl_* functions together for clarity.

ltalirz

thanks @unkcpz !

Looks good to me, just a few minor comments & requests.

Good to merge afterwards; also feel free to move the functions to the appropriate place now.

ltalirz · 2020-04-12T16:04:39Z

tests/cmdline/commands/test_data.py

@@ -343,6 +343,31 @@ def test_bandsexport(self):
        self.assertEqual(res.exit_code, 0, 'The command did not finish correctly')
        self.assertIn(b'[1.0, 3.0]', res.stdout_bytes, 'The string [1.0, 3.0] was not found in the bands' 'export')

+    def test_bandsexport_single_kp(self):
+        """
+        test for issue #2462: plot band on single kpoint case


Suggested change

test for issue #2462: plot band on single kpoint case

Plot band for single k-point (issue #2462).

ltalirz · 2020-04-12T16:05:34Z

tests/cmdline/commands/test_data.py

+            self.cli_runner.invoke(cmd_bands.bands_export, options, catch_exceptions=False)
+            with open('bands.gnu', 'r') as gnu_file:
+                res = gnu_file.read()
+                self.assertIn('vectors nohead', res, 'The string vectors nohead was not found in the gnuplot script')


Suggested change

self.assertIn('vectors nohead', res, 'The string vectors nohead was not found in the gnuplot script')

self.assertIn('vectors nohead', res, 'The string "vectors nohead" was not found in the gnuplot script')

ltalirz · 2020-04-12T16:28:41Z

aiida/orm/nodes/data/array/bands.py

@@ -885,7 +891,10 @@ def _prepare_gnuplot(self,
        """
        import os

-        dat_filename = os.path.splitext(main_file_name)[0] + '_data.dat'
+        if main_file_name is not None:


By the way, the default seems to be '' rather than None

I suggest to change the default of the function argument to None, and then:

if not main_file_name: # also catches '', just to be safe main_file_name = 'band.dat' # not sure what a reasonable default is dat_filename = os.path.splitext(main_file_name)[0] + '_data.dat'

Or even shorter, if you like:

main_file_name = main_file_name or 'band.dat' ...

the default filename is not important here, in fact it is not used. Default filename is only used when export gnuplot without specify the extra_file with -o, if that happened it will throw a command line critical information. Here the file_name is only to make sure os.path.splitext is working and throw the right Critical information to user.

aiida/orm/nodes/data/array/bands.py

unkcpz · 2020-04-12T18:07:09Z

Thanks for review! @ltalirz

ltalirz

Great, let's go!

plot energy levels when only 1 kpoints

9f51705

unkcpz mentioned this pull request Feb 24, 2020

verdi data bands export broken #2462

Closed

ltalirz self-requested a review February 24, 2020 15:05

ltalirz reviewed Feb 24, 2020

View reviewed changes

aiida/orm/nodes/data/array/bands.py Outdated Show resolved Hide resolved

aiida/orm/nodes/data/array/bands.py Outdated Show resolved Hide resolved

unkcpz added 4 commits February 26, 2020 17:50

seperate plot code by template

80d8935

move _prepare_mpl_* together

8c0785a

address problem

cedb309

add band gnuplot plot

651f44d

unkcpz commented Feb 27, 2020

View reviewed changes

tests/cmdline/commands/test_data.py Outdated Show resolved Hide resolved

unkcpz commented Feb 27, 2020

View reviewed changes

aiida/orm/nodes/data/array/bands.py Outdated Show resolved Hide resolved

unkcpz commented Feb 27, 2020

View reviewed changes

aiida/orm/nodes/data/array/bands.py Show resolved Hide resolved

unkcpz changed the title ~~WIP: plot energy levels when only 1 kpoints~~ [WIP]: plot bands with only one kpoint Feb 27, 2020

not break bands datalist test

637f826

unkcpz force-pushed the plot_energy_level branch from 1308fe4 to 637f826 Compare February 27, 2020 03:56

pre-commit

610db31

unkcpz force-pushed the plot_energy_level branch from 2a71e34 to 610db31 Compare February 27, 2020 04:25

unkcpz requested a review from ltalirz February 27, 2020 04:30

ltalirz suggested changes Feb 27, 2020

View reviewed changes

click file write test gnuplot

d6d7b76

unkcpz force-pushed the plot_energy_level branch from b664f93 to d6d7b76 Compare March 1, 2020 04:00

unkcpz mentioned this pull request Mar 10, 2020

isolated bandsdata list and test #3817

Merged

Merge branch 'develop' into plot_energy_level

c2599f5

unkcpz changed the title ~~[WIP]: plot bands with only one kpoint~~ plot bands with only one kpoint Apr 12, 2020

ltalirz suggested changes Apr 12, 2020

View reviewed changes

Fix issue aiidateam#2462: bands export error

68621fc

unkcpz force-pushed the plot_energy_level branch from f01760f to 68621fc Compare April 12, 2020 18:01

unkcpz requested a review from ltalirz April 12, 2020 18:14

ltalirz approved these changes Apr 12, 2020

View reviewed changes

ltalirz merged commit c74048b into aiidateam:develop Apr 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plot bands with only one kpoint #3798

plot bands with only one kpoint #3798

unkcpz commented Feb 24, 2020 •

edited by ltalirz

Loading

ltalirz left a comment •

edited

Loading

unkcpz commented Feb 25, 2020

ltalirz left a comment

ltalirz Feb 27, 2020

unkcpz Feb 27, 2020

unkcpz Feb 29, 2020

ltalirz Feb 27, 2020

unkcpz Feb 27, 2020 •

edited

Loading

ltalirz commented Apr 11, 2020

codecov bot commented Apr 11, 2020 •

edited

Loading

unkcpz commented Apr 12, 2020

ltalirz left a comment

ltalirz Apr 12, 2020

ltalirz Apr 12, 2020

ltalirz Apr 12, 2020 •

edited

Loading

unkcpz Apr 12, 2020

unkcpz commented Apr 12, 2020

ltalirz left a comment

	test for issue #2462: plot band on single kpoint case
	Plot band for single k-point (issue #2462).

	self.assertIn('vectors nohead', res, 'The string vectors nohead was not found in the gnuplot script')
	self.assertIn('vectors nohead', res, 'The string "vectors nohead" was not found in the gnuplot script')

plot bands with only one kpoint #3798

plot bands with only one kpoint #3798

Conversation

unkcpz commented Feb 24, 2020 • edited by ltalirz Loading

ltalirz left a comment • edited Loading

Choose a reason for hiding this comment

unkcpz commented Feb 25, 2020

ltalirz left a comment

Choose a reason for hiding this comment

ltalirz Feb 27, 2020

Choose a reason for hiding this comment

unkcpz Feb 27, 2020

Choose a reason for hiding this comment

unkcpz Feb 29, 2020

Choose a reason for hiding this comment

ltalirz Feb 27, 2020

Choose a reason for hiding this comment

unkcpz Feb 27, 2020 • edited Loading

Choose a reason for hiding this comment

ltalirz commented Apr 11, 2020

codecov bot commented Apr 11, 2020 • edited Loading

Codecov Report

unkcpz commented Apr 12, 2020

ltalirz left a comment

Choose a reason for hiding this comment

ltalirz Apr 12, 2020

Choose a reason for hiding this comment

ltalirz Apr 12, 2020

Choose a reason for hiding this comment

ltalirz Apr 12, 2020 • edited Loading

Choose a reason for hiding this comment

unkcpz Apr 12, 2020

Choose a reason for hiding this comment

unkcpz commented Apr 12, 2020

ltalirz left a comment

Choose a reason for hiding this comment

unkcpz commented Feb 24, 2020 •

edited by ltalirz

Loading

ltalirz left a comment •

edited

Loading

unkcpz Feb 27, 2020 •

edited

Loading

codecov bot commented Apr 11, 2020 •

edited

Loading

ltalirz Apr 12, 2020 •

edited

Loading