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

Tad reporting standard #93

Merged
merged 14 commits into from
Apr 12, 2024
Merged

Tad reporting standard #93

merged 14 commits into from
Apr 12, 2024

Conversation

ChristopherSpelt
Copy link
Contributor

TAD Reporting Standard

This PR contains the first version of the TAD reporting standard.

Reading order suggestions:

  • Read "introduction" and "intended usage"
  • Read the YAML example
  • Read the "specification of the standard"
  • Read the JSON schema

Expected shortcomings:

  • It is highly likely that we need to revise the standard to avoid very large yaml files. This explicitly has not been taken into account yet.
  • It is highly likely that we need to revise the datatypes.
  • It is highly likely that the way we capture measurements needs to be adjusted.

The shortcomings will probably be exposed at the moment we will try to actually write an example output, at which point we can address them.

@ChristopherSpelt ChristopherSpelt requested a review from a team as a code owner April 2, 2024 13:40
Copy link
Member

@anneschuth anneschuth left a comment

Choose a reason for hiding this comment

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

Very nice! Left some remarks. Some may be debatable or premature.

Copy link
Member

@berrydenhartog berrydenhartog left a comment

Choose a reason for hiding this comment

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

Some small comments

anneschuth
anneschuth previously approved these changes Apr 3, 2024
Copy link
Member

@anneschuth anneschuth left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@laurensWe laurensWe left a comment

Choose a reason for hiding this comment

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

In general this is great, the amount of details is just right and I think it captures everything that we would deem necessary for modelcards (both technical and assessmental) and it is flexibel enough to be extended by different graph measurements for example.

Copy link
Member

@robbertbos robbertbos left a comment

Choose a reason for hiding this comment

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

@ChristopherSpelt thanks for this initial proposal for the TAD reporting standard. With this review I hope to share some additional ideas, so we can make it even better. If you want to discuss some of my comments, please let me know.

@ChristopherSpelt ChristopherSpelt force-pushed the TAD-reporting-standard branch from 8ac1fae to e4b8696 Compare April 5, 2024 14:36
Copy link
Member

@laurensWe laurensWe left a comment

Choose a reason for hiding this comment

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

Just one minor remark, looks really good to me now :)

Copy link
Member

@robbertbos robbertbos left a comment

Choose a reason for hiding this comment

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

@ChristopherSpelt thanks for this new version. I left some comments and suggestions. I also propose to have a (short) meeting to talk about a couple of comments because I think that way we can align on them faster. I will bring it up during the standup tomorrow.

anneschuth
anneschuth previously approved these changes Apr 11, 2024
Copy link
Member

@anneschuth anneschuth left a comment

Choose a reason for hiding this comment

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

Amazing! 3 small ones

Copy link
Member

@robbertbos robbertbos left a comment

Choose a reason for hiding this comment

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

@ChristopherSpelt thanks for this new version. I think we should merge this version, so we can finish the first itteration. It was great working with you, and the team, on this!

There is one "bug" we should fix before we can complete this PR. The rest of my comments are just suggestions. Feel free to ignore them.

Copy link
Member

@robbertbos robbertbos left a comment

Choose a reason for hiding this comment

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

Looking good!

@robbertbos robbertbos merged commit 5e3e865 into main Apr 12, 2024
2 checks passed
@robbertbos robbertbos deleted the TAD-reporting-standard branch April 12, 2024 08:06
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.

5 participants