Skip to content

ENH: add global meta data#873

Merged
mj-will merged 13 commits into
bilby-dev:mainfrom
mj-will:add-global-meta-data
Feb 6, 2025
Merged

ENH: add global meta data#873
mj-will merged 13 commits into
bilby-dev:mainfrom
mj-will:add-global-meta-data

Conversation

@mj-will

@mj-will mj-will commented Dec 6, 2024

Copy link
Copy Markdown
Collaborator

Based on the discussion in #867, I've made a start on a possible implementation of a global meta data object.

For now, it's just a singleton dictionary that, by default, tracks the rng. Things like the cosmology can then be added when they're set.

Motivation

This change makes it easier to keep track of any global variables and allows us to ensure they're saved in the result file after running a sampler.

Changes (updated 27/01/25)

  • Add a new meta_data submodule and GlobalMetaData class
  • Add functions for encoding/decoding numpy random generators
  • Add test for saving generators
  • Extend seed, set_cosmology and _set_default_cosmology to update the global meta data when called.
  • Add functions for encoding UserDict and UserList subclasses. These rely on the fact both classes have a data attribute, see the Python docs

Testing

These changes are mostly covered by the existing tests save result files, but I added a test for the encoding/decoding change and for the global meta data class.

@mj-will mj-will added the enhancement New feature or request label Dec 6, 2024

@ColmTalbot ColmTalbot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks like a good start to me.

Comment thread bilby/core/utils/random.py Outdated
from .meta_data import global_meta_data

Generator.rng = default_rng(seed)
global_meta_data.add_to_meta_data("rng", Generator.rng)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is using this method instead of just doing global_meta_data[key] = value required, or some convention I haven't come across before?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I thought we may want to add some form of check/validation when adding values, so this would make it easier in future. Happy to revert to something simpler though

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You can in principle override the get/set item: https://stackoverflow.com/questions/2390827/how-to-properly-subclass-dict-and-override-getitem-setitem

However, my experience when doing this before is that can introduce unexpected behaviour. But, on the other hand it may be a good idea so that any checks are uniformly applied (e.g. if someone utilises the get/set method instead of using add_to_metadata

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That's a good idea @GregoryAshton. I've used UserDict as the base class, since this is less error-prone than inheriting directly from dict. This does mean, however, I've had to add encodings for UserDict (I add UserList as well since they can share the same if statement).

Let me know what you both think

Comment thread bilby/core/utils/random.py
@mj-will

mj-will commented Dec 9, 2024

Copy link
Copy Markdown
Collaborator Author

The tests are going to fail until #867 is merged, since it adds support for properly saving the cosmology objects

@mj-will mj-will added this to the 2.5.0 milestone Jan 9, 2025
@mj-will mj-will force-pushed the add-global-meta-data branch from aabeafd to 45fbd9e Compare January 23, 2025 17:02
@mj-will

mj-will commented Jan 24, 2025

Copy link
Copy Markdown
Collaborator Author

In what seems to be a reoccurring theme, I realised that adding the rng to the result object requires new encoding/decoding functions. For now, I've added them here but I'm happy to move them to another PR.

@mj-will mj-will force-pushed the add-global-meta-data branch from b788d37 to 1a808c9 Compare January 24, 2025 12:46
@mj-will mj-will marked this pull request as ready for review January 24, 2025 13:18
@mj-will mj-will force-pushed the add-global-meta-data branch from a1a1a72 to a93dea1 Compare January 27, 2025 20:41

@ColmTalbot ColmTalbot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks good to me now.

Comment thread bilby/core/utils/random.py

@asb5468 asb5468 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks good to me and the tests cover the new functionality suitably.

@mj-will mj-will force-pushed the add-global-meta-data branch from a93dea1 to d545b5b Compare February 4, 2025 18:25
@mj-will mj-will merged commit a195251 into bilby-dev:main Feb 6, 2025
@mj-will mj-will deleted the add-global-meta-data branch February 6, 2025 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request >100 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants