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 a whole bunch of default values #59

Merged
merged 10 commits into from
Apr 1, 2024
Merged

Conversation

sarthakpati
Copy link
Collaborator

@sarthakpati sarthakpati commented Mar 31, 2024

I am pretty sure there is a better way to do this (for example, having a base Config class under ereg.configurations that reads the sample_config.yaml and populates a baseline Config object, which can then be used in the RegistrationClass class.

However, I unfortunately don't have the kind of time on hand right now to make this change. I would really appreciate it if someone could help me put this together.

Includes #57, so that should/will get merged along with this.

@sarthakpati sarthakpati linked an issue Mar 31, 2024 that may be closed by this pull request
@sarthakpati sarthakpati requested a review from neuronflow March 31, 2024 16:30
@brainless-bot
Copy link
Contributor

brainless-bot bot commented Mar 31, 2024

🤖 Code Formatting Reminder

Hello there! 👋 It looks like the code in this pull request might benefit from some formatting improvements.
Fix the issues locally or use our auto format action by commenting /format on this PR!

Code style: black

@neuronflow
Copy link
Contributor

/format

@brainless-bot
Copy link
Contributor

brainless-bot bot commented Mar 31, 2024

🤖 I will now format your code with black. Check the status here.

@neuronflow neuronflow added the enhancement New feature or request label Mar 31, 2024
@neuronflow
Copy link
Contributor

neuronflow commented Mar 31, 2024

I am pretty sure there is a better way to do this (for example, having a base Config class under ereg.configurations that reads the sample_config.yaml and populates a baseline Config object, which can then be used in the RegistrationClass class.

However, I unfortunately don't have the kind of time on hand right now to make this change. I would really appreciate it if someone could help me put this together.

can you open a feature request so we don't lose track of this? @sarthakpati

@sarthakpati sarthakpati marked this pull request as draft March 31, 2024 16:34
@neuronflow
Copy link
Contributor

@sarthakpati currently, unit tests are not passing yet.

@neuronflow
Copy link
Contributor

neuronflow commented Mar 31, 2024

once this passes unit tests and is merged, we can merge #57 to require passed tests before merging, so we don't break ereg again :)

Copy link
Contributor

@neuronflow neuronflow left a comment

Choose a reason for hiding this comment

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

How do you feel about putting default values in a yaml file to create a single source of truth?

@sarthakpati
Copy link
Collaborator Author

sarthakpati commented Mar 31, 2024

How do you feel about putting default values in a yaml file to create a single source of truth?

Sure, but let's tie it in with #60. Doing yaml I/O in a purely computational class is just clunky.

@brainless-bot
Copy link
Contributor

brainless-bot bot commented Mar 31, 2024

🤖 Code Formatting Reminder

Hello there! 👋 It looks like the code in this pull request might benefit from some formatting improvements.
Fix the issues locally or use our auto format action by commenting /format on this PR!

Code style: black

@sarthakpati sarthakpati marked this pull request as ready for review April 1, 2024 12:54
@neuronflow neuronflow merged commit 7fe8fa4 into main Apr 1, 2024
6 checks passed
@neuronflow neuronflow deleted the 56-keyerror-metric branch April 1, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KeyError: 'metric'
2 participants