Skip to content
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

Improvements to Report files tab #15

Open
dch0ph opened this issue Feb 9, 2023 · 5 comments
Open

Improvements to Report files tab #15

dch0ph opened this issue Feb 9, 2023 · 5 comments

Comments

@dch0ph
Copy link

dch0ph commented Feb 9, 2023

Some quick critiques on the Report files tab, just looking at shielding for now:

image

I wouldn't put "Select and display" on its own line - bold does the highlighting.

"File type" -> "Output" [you're not changing the file type]
Below I would have "Options:" and move "Include" to "Euler angles"

"Merge equivalent labels" appears 3 times. I would have one tick box with a tool tip. It's not obvious what would happen if the file didn't have crystallographic labels?

If merging, there should be a column for “Multiplicity”. Particularly for inorganic systems the multiplicity will be site dependent.

Would "export as shifts" be available if referencing were set?

In the file itself:
image

The output seems to be based on fixed columns, which is horrible. I think tab separated is a sensible default, since it's vaguely readable. You could have comma separated as an option (I don't think , is likely in output).

The comment lines must be preceded by a comment character. # is a sensible choice (default for numpy).

A more informative comment would be "MS table generated by MagresView 2 from " (assuming shielding output). Including any parameters e.g. referencing used gives full reproducibility.

Element -> "Isotope". Although if you are outputting isotope-independent properties, I think it is smarter to output element (i.e. Z).

I don't understand the index column. Is this based on the ASE index? This will make no sense to the user. Any index should reflect the original input.

Similarly I would only use "Label" if these are user-labels.

@jkshenton
Copy link
Member

jkshenton commented Feb 15, 2023

Thanks for the feedback! To help keep track of these I'm going to be editing this reply as I address them.

  • I wouldn't put "Select and display" on its own line - bold does the highlighting.

  • "File type" -> "Output" [you're not changing the file type]

  • Below I would have "Options:" and move "Include" to "Euler angles"

  • "Merge equivalent labels" appears 3 times...

  • If merging, there should be a column for “Multiplicity”.

  • Would "export as shifts" be available if referencing were set?

  • The output seems to be based on fixed columns, which is horrible.

    • add CSV output option
    • add tab separated option (default?)
  • The comment lines...

  • A more informative comment would be...

  • Element -> "Isotope" ...

I don't understand the index column. Is this based on the ASE index? This will make no sense to the user. Any index should reflect the original input.

The index is the same as that in the original input file (the ASE index is just that minus one).

Similarly I would only use "Label" if these are user-labels.

I think the MV2-generated labels (generated in cases where the loaded file doesn't have CIF-style labels) are useful to compare what the user sees in the GUI (they can show these labels in the Select and Display sidebar) with what's in the output file. Would you recommend changing the column header to 'MV2-generated label' or similar in such cases?

@jkshenton
Copy link
Member

One additional question I have is about the multiplicity. At the moment, the files output is always determined by the current selection of atoms. When calculating the multiplicity, is it safe to assume that that user wants the multiplicity within the current selection, or should we instead give the 'global' multiplicity (i.e. how many times each label appears in the loaded file, regardless of the current selection)?

At the moment I assume the user wants the multiplicity within the selection and have added a tooltip explaining this. If no atoms are selected then all atoms are exported and the 'global' multiplicity is given.

@dch0ph
Copy link
Author

dch0ph commented Feb 15, 2023

The index is the same as that in the original input file (the ASE index is just that minus one).

But there is no explicit index in the .magres? The index should match the "number in species label":
image

I don't know whether ASE guarantees that the order in Atoms matches the order the in the magres file, but in any case the order of atoms in .magres is not defined.

So the ASE index / "order in input file" is not a well defined concept.

When calculating the multiplicity, is it safe to assume that that user wants the multiplicity within the current selection, or should we instead give the 'global' multiplicity (i.e. how many times each label appears in the loaded file, regardless of the current selection)?

I would only have a multiplicity column if "Reduce" is selected, with the multiplicity being directly related to the reduction.

So yes, it would be "multiplicity within current selection" (which might be a weird thing, but would be Least Surprising).

I think the MV2-generated labels (generated in cases where the loaded file doesn't have CIF-style labels) are useful to compare what the user sees in the GUI

Good point. The MV2-generated labels are deliberately different from CIF-style labels, so I feel that a simple "Label" column for either generated or supplied is fine.
.

@jkshenton
Copy link
Member

Apologies for the confusion. The index, as it currently stands, is simply a counter following the order of the atoms in the .magres file. i.e. index 1 is the first atom listed in the magres file and atom 47 is the 47th etc.

It's more tricky in the case of CIF files having more than P1 symmetry -- then the indices are determined by the application of the symmetry operations (when the file is loaded, the symmetry is essentially reduced to P1). This is only potentially problematic for outputting dipolar couplings of such files.

I'll switch to using "number in species label".

@dch0ph
Copy link
Author

dch0ph commented Mar 16, 2023

Hi @jkshenton . Some additional points following more usage.

I don't think the "File type" thing has been changed (I was checking on your development version)? I would also change the menu entries to "MS", "EFG" etc. i.e. drop the unnecessary "table".

Merge equivalent labels should be greyed out / replaced with a message about no merging being possible if there are no labels. [I would have this a key flag associated with a structure (it is not going to change)]. Otherwise you have a tick item that does nothing.

You should be able to export as shifts - it is odd being able to plot shifts but not export them. Either this could be another menu option (probably more elegant), where it is an error to try to export shifts for elements with undefined references, or it could be an additional column with blank entries where no reference has been set. CSV is happy with blank entries, but it would create import complications e.g. numpy will not like heterogeneous data.

jkshenton added a commit to jkshenton/magresview-2 that referenced this issue Mar 16, 2023
- consolidated options in sidebar
- option to merge by CIF labels if present
- csv or fixed width option
- precision control

Partially addressed issue:
CCP-NC#15
jkshenton added a commit to jkshenton/magresview-2 that referenced this issue Mar 16, 2023
- labels and indices now match the .magres file
- Indices correspond to the "number in species label" from the .magres

Partially addresses issue:
CCP-NC#15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants