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

Alternative method to Utils optional options #169

Merged

Conversation

GeoDerp
Copy link
Contributor

@GeoDerp GeoDerp commented Feb 1, 2024

No description provided.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Feb 1, 2024

@davidusb-geek
You can pick one or the other. (other being #167)
Personally I like this approach more as its easy to read. However mat be complex to understand.

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (6fe81cf) 88.54% compared to head (12f1be5) 89.96%.

Files Patch % Lines
src/emhass/forecast.py 33.33% 2 Missing ⚠️
src/emhass/utils.py 98.59% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #169      +/-   ##
==========================================
+ Coverage   88.54%   89.96%   +1.41%     
==========================================
  Files           6        6              
  Lines        1668     1644      -24     
==========================================
+ Hits         1477     1479       +2     
+ Misses        191      165      -26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@davidusb-geek
Copy link
Owner

This option seems much more elegant. Nice work.

@davidusb-geek
Copy link
Owner

Both solar.forcast and solcast should be broken. That was the main reason for both pull requests initially. Would have to check if that's related to that comment however.

The bugs from this comment are also solved on this PR.
In other words, if we use this PR instead of #167, the bugs on solar.forcast and solcast are solved?

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Feb 3, 2024

In testing it seemed to fix the issue also. Will do a double check now to be double sure.
(But yeah if you pull this one we will close the other)

@davidusb-geek
Copy link
Owner

I think that we will push up this method, if everything works correctly.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Feb 3, 2024

Tested, looks like everything works. Actually found an bug when setting solar_forecast_kwp to 0 so added a patch fix (feel free to revert once pulled)

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Feb 3, 2024

hangon. may of found another bug. Looking at it now.
Having an issue with new_string = string.replace(var_load, var_load+'_positive') when I delete all parameters in options.json
Trying to work out why.

Edit: I think I know why

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Feb 3, 2024

@davidusb-geek Should be good to go now 👍
My most tested code yet (not saying its perfect)

@davidusb-geek
Copy link
Owner

Great! +1

params['plant_conf']['SOCmax'] = options.get('battery_maximum_state_of_charge',params['plant_conf']['SOCmax'])
params['plant_conf']['SOCtarget'] = options.get('battery_target_state_of_charge',params['plant_conf']['SOCtarget'])

params['associations_dict'] = associations_dict
Copy link
Owner

Choose a reason for hiding this comment

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

So why save this association dict, it will be used elsewhere?

Copy link
Contributor Author

@GeoDerp GeoDerp Feb 3, 2024

Choose a reason for hiding this comment

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

We do. For testing. We check that options parameters are properly overriding the default config.
My method so far to make sure something didnt go crazy in the commit.

@davidusb-geek davidusb-geek merged commit d37aac3 into davidusb-geek:master Feb 3, 2024
13 checks passed
GeoDerp added a commit to GeoDerp/emhass that referenced this pull request Feb 5, 2024
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.

2 participants