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

Added generalisation of png metadata handler #1199

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

WillTrojak
Copy link
Contributor

This is in answer to #1155.

The intention is to generalise the handling of setting metadata in the PNG files. I am open to changes in the syntax that I've used or which domain is chosen to set the metadata from.

At the moment, the syntax that I've used is that for the first child domain, any child of the state node that has a child called keyword and data will create a png metadata keyword. ie

domain_0:
   state:
      domain_id: 0
      time: 
         keyword: 'Time'
         data: 0.1
domain_1:
   state:
      domain_id: 1
      comment: 
         keyword: 'Comment'
         data: 'some comment'

Will give the png metadata:

Time: 0.1

This seemed like the cleanest way of handling this.

@WillTrojak WillTrojak changed the title Added generialisation of png metadata handler Added generalisation of png metadata handler Sep 8, 2023
@cyrush
Copy link
Member

cyrush commented Sep 8, 2023

@WillTrojak good catch and nice suggestion!

They way ascent is currently plumbed, PNG comments are only used MPI task 0, (since task 0 writes the final pngs)

For the general case, it's possible that task 0 may have no domains at all. This is rare but we need to support it.

So we will need an extension to your code to find the first task that has this info, and then get that info to Task 0 (or all tasks).

That will require some MPI coordination. I can help implement this, but won't have time until later next week.

@FreddieWitherden
Copy link

For the general case, it's possible that task 0 may have no domains at all. This is rare but we need to support it.

This is quite likely in the PyFR use case as we often only pass Ascent a small subset of the total computational domain. As such it is not at all unusual for ranks to be excluded and have no data of their own (but they all make collective calls to Ascent).

@WillTrojak
Copy link
Contributor Author

I added a simple test so that the first domain that has metadata is used. Let me know if you forsee any issues with this.

Copy link
Member

@cyrush cyrush left a comment

Choose a reason for hiding this comment

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

Yes, this looks good. I think we should get this in now.

Sorry for the delay on the MPI version of the checks.
I can follow up with those changes later.

@cyrush cyrush merged commit 883aef9 into Alpine-DAV:develop Oct 5, 2023
18 of 20 checks passed
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.

3 participants