search

CCP-NC / magresview-2

MagresView 2.0 - NMR crystallography visualisation app
https://ccp-nc.github.io/magresview-2/
MIT License
0 stars 2 forks source link

Improvements to Report files tab #15

Open dch0ph opened 1 year ago

dch0ph commented 1 year ago

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 commented 1 year ago

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

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.