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

merge dt changes #815

Merged
merged 221 commits into from
Jan 20, 2022
Merged

merge dt changes #815

merged 221 commits into from
Jan 20, 2022

Conversation

jd-lara
Copy link
Member

@jd-lara jd-lara commented Jan 3, 2022

Opening a PR because this branch is already pretty large. Check the comments for the pending development functions.

@jd-lara jd-lara requested a review from daniel-thom January 20, 2022 18:38
src/operation/emulation_model_store.jl Outdated Show resolved Hide resolved
src/operation/emulation_model_store.jl Show resolved Hide resolved
end

function read_optimizer_stats(store::InMemorySimulationStore, model_name)
# TODO EmulationModel: this interface is TBD
Copy link
Contributor

Choose a reason for hiding this comment

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

Does something need to change here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this is not implemented yet. I need to do this in a separate PR with the caching.

src/simulation/simulation.jl Outdated Show resolved Hide resolved
src/simulation/simulation.jl Outdated Show resolved Hide resolved
for field in fieldnames(ValueStates)
model_params = get_model_params(store, model_name)
model_params = get_decision_model_params(store, model_name)
for field in fieldnames(DatasetContainer)
for key in list_fields(store, model_name, field)
# TODO: Read Array here to avoid allocating the DataFrame
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this TODO still apply? I guess it's tied to my other comment about making a non-copy read_results function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it does apply. But after testing I wonder how much it matters.

result_length = length(first_id:last_id)
for colname in propertynames(df)
colname == :DateTime && continue
# TODO DT: may not be correct
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we confirm this and then delete the comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't confirm it is correct at the moment. @claytonpbarrows was assigned the validation. We should leave the comment and merge to dev for further testing of the results code.

src/utils/jump_utils.jl Outdated Show resolved Hide resolved
@jd-lara jd-lara requested a review from daniel-thom January 20, 2022 20:03
@jd-lara jd-lara merged commit 58884bb into dev Jan 20, 2022
@jd-lara jd-lara deleted the jd/merge_dt_changes branch January 20, 2022 20:14
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