Skip to content

Second step in an attempted cleanup of the CLI part - reverse the index order to make xyz the standard#378

Open
mondracek wants to merge 9 commits intomainfrom
reverse
Open

Second step in an attempted cleanup of the CLI part - reverse the index order to make xyz the standard#378
mondracek wants to merge 9 commits intomainfrom
reverse

Conversation

@mondracek
Copy link
Collaborator

@mondracek mondracek commented Mar 12, 2026

This PR is a follow-up to #376, another step to gradually address #198, and the main part of replacement for the failed #347.

  • As the title says, the primary point is to implement the [ix,iy,iz] index order (in C-like memory arrangement) for arrays that represent grid data in the CLI part of the code.
  • An exception are the plotting functions in ppafm/PPPlot.py, which need the [iz,iy,ix] order internally. These function now transpose the input array(s) at the beginning, so they still expect the new standard of the [ix,iy,iy] order on their input. Moreover, since the change in indexing has made it difficult to specify the slices argument such that it would cover the whole array, a new default of slices=None has been implemented, which does exactly this: if slices is None: then plot all the slices.
  • The changes in the required index order for the input of the ppafm/PPPlot.py functions can be suppressed by a newly added option xyz_order=False (see also the next comment of mine).
  • As a side effect, this PR probably solves The option to rotate sample in the *xy*-plane does likely not work correctly #346. I am not sure, because we actually never use the --rotate option (with which that issue is concerned) for anything, so I have not tested it.
  • Finally, as a complete aside, I have spotted a typo in a parameter name that happened to appear near the parts of the code I was changing for this PR: pixPerAngstrome with the extra "e" at the end. So I have changed this to pixPerAngstrom everywhere.

Note: The bug that prevented the ocl version from working in #347 was likely the index reordering by array.T in function subCores of ppafm/ocl/field.py, which I failed to remove here and here.

 * Rename `loadXSF` to `loadXSFData` and `saveXSF` to `saveXSFData`
   to make the original names available for the furture new functions.
 * Add the `xyz_order` option to `saveXSFData` arguments.
 * Remove functions `loadVecFieldXsf`, `loadVecFieldNpy`, `saveVecFieldXsf`, `saveVecFieldNpy`
   to simplify the io.py code structure.
 * by removing array transpositions in ocl.field.TipDensity.subCores()
 * and reinterpreting nDim in common.getPos()
 * by correcting the definition of `zpos` in ppafm/cli/generateElFF.py,
 * making `xyz_order` the default for loadCUBE in ppafm/io.py,
 * and fixing the `rotate` functionality in ppafm/cli/relaxed_scan.py
@mondracek
Copy link
Collaborator Author

mondracek commented Mar 12, 2026

I have realized that I should add an xyz_order option to functions plotImages, plotVecFieldRG, plotDistortions and plotArrows in ppafmPPPlot.py, so that you can recover the old behaviour of those functions by setting xyz_order=False. Also, I need to add some forgotten slices=None defaults and fix a wrong transposition in checkVecField, so please wait till I push the commit that would take care of this. Ready now.

@mondracek
Copy link
Collaborator Author

Further notes (about writing and reading data files):

  • I have made xyz_order=True the default for loadXSF, saveXSF and loadCUBE.
  • Moreover, I have removed functions loadVecFieldXsf, saveVecFieldXsf, loadVecFieldNpy, saveVecFieldNpy from ppafm/io.py, in order to establish a direct (and therefore more transparent) connection of the lower-level loadXSF and loadNpy to the higher-level load_vec_field, as well as of saveXSF to save_vec_field.
  • I intend to do a more thorough rewriting of the input from and output to the data files in a future pull request anyway.

@mondracek mondracek requested a review from NikoOinonen March 12, 2026 15:58
@mondracek mondracek self-assigned this Mar 12, 2026
Copy link
Collaborator

@NikoOinonen NikoOinonen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Martin, everything looks fine to me except that I would rather not rename the PixPerAngstrome argument right now.

def __init__(
self,
pixPerAngstrome=10,
pixPerAngstrom=10,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better not to change this right now. I have probably a hundred scripts all around that use the existing naming, and some of Adam's guys have been using it too, so breaking it without notice is not good. There is also another issue for overhauling the arguments here more thoroughly (#338), so I would leave this for later.

Comment on lines +589 to +590
if xyz_order:
data = data.transpose((2, 1, 0))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of the argument xyz_order feels a bit unintuitive to me here. When I first read it, I expected it to mean that the output will be saved in xyz order, but actually it means that the input is expected in xyz order. Maybe a bit more explicit name like data_is_xyz_order would be better?

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

Successfully merging this pull request may close these issues.

2 participants