Skip to content

Merged changes to peaks/dev into peaks_AndersDev, and added the sgm4 …#16

Merged
shum23 merged 5 commits into
phrgab:devfrom
AndersSMortensen:main
Apr 23, 2026
Merged

Merged changes to peaks/dev into peaks_AndersDev, and added the sgm4 …#16
shum23 merged 5 commits into
phrgab:devfrom
AndersSMortensen:main

Conversation

@AndersSMortensen
Copy link
Copy Markdown
Contributor

@AndersSMortensen AndersSMortensen commented Apr 17, 2026

Merging the sgm4 loader into the peaks dev branch.

This was referenced Apr 18, 2026
@shum23 shum23 added the enhancement New feature or request label Apr 22, 2026
@shum23
Copy link
Copy Markdown
Collaborator

shum23 commented Apr 22, 2026

Thanks @AndersSMortensen for the contribution and changes and sending some test data.

Note: the following has been fixed in commit c2188e8. Putting discussions here for future reference.

  • The timestamp seems to exist in the data files under the key 'Entry/Data/Timestamp', but the timestamp that shows in the metadata is the file creation timestamp on the system, which is the fallback in peaks​ when no timestamp is available in the data. It would be great to map the initial timestamp from the data (i.e. the start time) to the metadata.scan.timestamp​ field
  • metadata.scan.loc​ is SGM4 now. To be consistent with the general naming conventions used elsewhere in peaks​, would you be able to rename it to e.g. ASTRID2_SGM4
  • analyser.scan.eV​ always calculates the centre kinetic energy. This is great for fixed mode scans, but for swept scans, it could be helpful to show the energy range as a two-value array [E_start, E_end] as is implemented in other peaks​ loaders?
  • You have a scan with a 'Loop' co-ordinate. If we understand correctly, that is basically a repetition of the same scan just at e.g. a later point in time, making multiple iterations. This behaviour is already encoded elsewhere in peaks​ using a dimension name of scan_no ​(see, e.g. _desired_dim_order​ of class:BaseARPESDataLoader​, as e.g. used in the SES data loader. Given that this seems really the same functionality here, it would be helpful if we could maintain the scan_no​ co-ordinate/dim name for this too, if possible (I think should just be renaming Loop to scan_no)?
  • We had some complications over the sign conventions for loaders before. So can we just double check, for 'WSe2Cold_4_0' or 'WSe2Cold_7_0', should the actual normal emission be approximately:
    manipulator polar: -26 deg
    tilt: -9 deg
    azi: 10 deg if aligned with gamma-M
    The key question there is whether the signs for those are as you would expect for this data set or are inverted.
  • A trivial thing, but there are a couple of lines commented out in the loader file, I guess used for debugging or inherited from the diamond loader. If you have a chance, might be good to clean them.

@shum23
Copy link
Copy Markdown
Collaborator

shum23 commented Apr 22, 2026

Hi @AndersSMortensen,

Thanks. I think this looks great and is ready to merge. I've just noticed a couple of wee typos about the Keithley parameters. Around line 633 in sgm4.py, KA_V seems to map to KeithleyA_Resistance while on line 116 it's pulling the keithleyA_Voltage metadata. I reckon it means the voltage there, and the same for KA_R, KB_V, and KB_R. If you could fix these typos that'd be great.

And would you also be able to add an entry for this new loader in the changelog?

@AndersSMortensen
Copy link
Copy Markdown
Contributor Author

Thank you @shum23! I have fixed the typos and added a comment in the changelog.
I have just written that the loader is there. If there is any key information you would like me to add, let me know.

@shum23 shum23 self-requested a review April 23, 2026 09:31
Copy link
Copy Markdown
Collaborator

@shum23 shum23 left a comment

Choose a reason for hiding this comment

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

Thanks @AndersSMortensen. Merging now!

@shum23 shum23 merged commit 66fa831 into phrgab:dev Apr 23, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants