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

YAN-858 Frequency Scattering of Visibilities #94

Merged
merged 7 commits into from
Jan 21, 2022
Merged

YAN-858 Frequency Scattering of Visibilities #94

merged 7 commits into from
Jan 21, 2022

Conversation

calgray
Copy link
Collaborator

@calgray calgray commented Jan 11, 2022

As part of sdp work there is a need to split measurement set data on by frequency for daliuge parallel processing. From previous MS extraction I've used .npy serialized format and created a scatter app that can split multiple input ndarrays on the frequency axes specified by list param "Scatter Axes".

In implementing the scatter app I've noticed input app parameters where not being sent to the engine so I've explicitly populated input app fields for the logical graph such that the scatter app can be processed to the physical graph like a regular in python app. Additionally I send fields from the parent scatter node to the input app so the scatter number is always correct.

image

@coveralls
Copy link

coveralls commented Jan 11, 2022

Coverage Status

Coverage increased (+0.2%) to 79.429% when pulling edcbbca on YAN-858 into cc15b7d on master.

@calgray calgray changed the base branch from LIU-213 to master January 14, 2022 06:55
@calgray
Copy link
Collaborator Author

calgray commented Jan 17, 2022

2 seperate generic scatter apps is fine temporarily, however with the update behavior and parameter changes it should be possible to combine all the features of generic scatter app by adding a 'use_pickle' flag to the params list. Adding these to GenericScatterApp will break existing scatter graphs so an upgrade plan would be needed. I'd suggest either:

  • manually update all repo graphs
  • a .graph update tool
  • support deprecated variables and raise warning

I'd lean towards deprecated warnings that print how to upgrade (namely scatter_axis -> scatter_axes, add use_pickle) and some effort towards manually updating EAGLE-graph-repo.

@awicenec
Copy link
Contributor

The main part of the changes are fine, but we have to address the changes in dm_utils.py in a different way. The reason for the splitting of the fields into the additional appFields was to enable paasing those parameters on to the actual application. The other fields are passed to the drop. This is required in particular for dockerized applications which require command line parameters. Thus I think we need to make sure that the translator is passing on those fields, but still enables the engine to distinguish them. I guess we could add an additional flag to the fields, or we could keep them entierly separate like they are coming from EAGLE. Since this is not really related to this current pull request I will still accept the changes.

@awicenec awicenec merged commit cadb548 into master Jan 21, 2022
@awicenec awicenec deleted the YAN-858 branch January 21, 2022 03:35
if INPUT_APP_FIELDS in node:
# inputAppFields are converted to fields to be processed like
# regular application drops
app_node["fields"] = list(node[INPUT_APP_FIELDS])
Copy link
Collaborator Author

@calgray calgray Jan 21, 2022

Choose a reason for hiding this comment

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

This line I found is needed since app parameters are meant to exist at under "fields" in the logical graph. These fields are from the app view in the inputApplication dropdown. The other fields I think some careful consideration needs to be made about. A logical graph schema may help a lot.

awicenec added a commit that referenced this pull request May 19, 2022
YAN-858 Frequency Scattering of Visibilities
pritchardn pushed a commit that referenced this pull request May 20, 2022
YAN-858 Frequency Scattering of Visibilities
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.

None yet

4 participants