Skip to content

Conversation

@macintoshpie
Copy link
Contributor

Adds ability to export 3 and 4 parameter models into bsync.
5P is not handled b/c it is failing to complete with our SEED test data currently.

Copy link
Contributor

@corymosiman12 corymosiman12 left a comment

Choose a reason for hiding this comment

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

Minor changes - seems simple enough.

"Daily" = "Day",
"Hourly" = "Hour",
"SLR" = "2 parameter simple linear regression",
"Three Parameter Heating" = "3 parameter heating change point model",
Copy link
Contributor

Choose a reason for hiding this comment

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

@macintoshpie should we remove the redundant ones? i.e. I assume the keys with 'Three Parameter Heating' and '3PH' are the same, and you are probably only using '3PH'...unless I missed something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need both from what I can tell b/c it depends on what you pass to nmecr (which allows both)

model <- nmecr::model_with_CP(bsyncr_df,
                                nmecr::assign_model_inputs(regression_type = "Three Parameter Cooling"))
print(model$model_input_options$regression_type)
# prints "Three Parameter Cooling"
model <- nmecr::model_with_CP(bsyncr_df,
                                nmecr::assign_model_inputs(regression_type = "3PC"))
print(model$model_input_options$regression_type)
# prints "3PC"

Copy link
Contributor

@corymosiman12 corymosiman12 left a comment

Choose a reason for hiding this comment

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

Looks good. Anyway to add some testing? Or just add an issue to begin define testing? I think we can probably just have a predefined:

  • Input file
  • Output file (expected)

And then the test just compares a new output file to the expected output file. It can be more 'outcome based' than unit test based - make sense?

@macintoshpie
Copy link
Contributor Author

yeah we should definitely add some testing at some point

@macintoshpie macintoshpie merged commit 63da964 into develop Jan 12, 2021
@macintoshpie macintoshpie deleted the feat/models branch January 12, 2021 17:37
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