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

Fix issues with calculate_qw_pixels2 for recent horace #1751

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

oerc0122
Copy link
Collaborator

@oerc0122 oerc0122 commented Sep 24, 2024

Update function to work with unique_references_container, struct-array header and use established functions.

This will likely be used in the on-the-fly re-computation of pixels.

Fixes #1152

@oerc0122 oerc0122 added the bug Something isn't working label Sep 24, 2024
@oerc0122 oerc0122 self-assigned this Sep 24, 2024
@oerc0122 oerc0122 marked this pull request as ready for review September 25, 2024 14:46
Copy link
Member

@abuts abuts left a comment

Choose a reason for hiding this comment

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

Just comment:
solves the problem so no direct objections but quality of changes is low as opportunity to remove substantial and very important code duplication is missed.

Comment on lines 42 to +43
if ~iscell(win.header)
header = {win.header};
header = num2cell(win.header)';
Copy link
Member

Choose a reason for hiding this comment

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

To be fair, we should not use header in new code. This is legacy method, necessary for writing old format sqw files and may be in some printing (also legacy, no time to throw it from there)

This is probably another ticket though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What is the new alternative?

Copy link
Member

Choose a reason for hiding this comment

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

Experiment.IX_experiment or sqw.experiment_info.expdata

Comment on lines +96 to 99
function detdcn = spec_coords_to_det (detpar)
% Matrix to convert coordinates in spectrometer (or laboratory) frame into detector frame
%
% >> d_mat = spec_coords_to_det (detpar)
Copy link
Member

Choose a reason for hiding this comment

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

This is code duplication as something similar is in rundatah/convert_to_sqw. Which one is better I do not know, as see some optimization here too. Probably other ticket again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A whole chunk of this including calculate_qw_pixels2 is duplicated in rundatah (c.f. rundatah.calc_qspec) and the whole thing needs to be split off into utilities at some point, but like you say, not this ticket.

@abuts abuts assigned abuts and unassigned oerc0122 Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing tests for calculate_qw_pixels2
3 participants