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

Dynamic paths with emhass config dict, some mlforcaster error suppression #247

Merged
merged 17 commits into from
Apr 18, 2024

Conversation

GeoDerp
Copy link
Contributor

@GeoDerp GeoDerp commented Mar 29, 2024

Will do more checking tomorrow. Here early for progress and help.

Added Dictionary var containing EMHASS paths

emhass_config dictionary has been created (currently, containing data and config paths) to try and add directory structure consistency though the program.

Currently, if the data path is outside the root EMHASS directory, certain errors may occur from some hard coded paths.
emhass_config tried to change this by passing in a dictionary to (I believe) all sub models.

MLForcaster error suppression

Added some more error suppression. (more testing should be done)
MLForcaster wise, I'm still not supper confident with how MLForcaster works. My assumption is, if there is missing data in the sensor, that days worth gets rejected, Causing the "ValueError: All arrays must be of the same length" error. (between forecast_dates and forecast_out.).
I have added an if statement to notify the user of the issue, replacing the ValueError. This however doesn't fix the issue.

src/emhass/command_line.py Fixed Show fixed Hide fixed
src/emhass/command_line.py Fixed Show fixed Hide fixed
src/emhass/command_line.py Fixed Show fixed Hide fixed
@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 29, 2024

The pytests are failing with (I believe) test_main_publish_data. It's a weird issue (shouldn't work but does work) , will look into tomorrow.

@davidusb-geek, If you could have a look over this at some point that would be amazing. (ex. Check I didn't place the wrong directory)

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 30, 2024

Looks like there are a few more issues then I first realized. Unfortunately Its difficult to understand the current pytest errors.

ValueError: The provided timedelta will relocalize on a nonexistent time: 0 days 00:30:00

Error, the randomly pops up

ValueError: Shape of passed values is (25, 1), indices imply (23, 1)

.+ the test_main_publish_data error.

@davidusb-geek
Copy link
Owner

ValueError: The provided timedelta will relocalize on a nonexistent time: 0 days 00:30:00

This may be related to the current errors of DST change

@davidusb-geek
Copy link
Owner

I'm still trying to understand the changes proposed in this PR...

@davidusb-geek
Copy link
Owner

@GeoDerp hopefully you can help with a current issue that I have relating to paths.
I added two new database files for updated PV modules and inverters. The files are here: https://github.com/davidusb-geek/emhass/tree/master/src/emhass/data
But when testing using the docker standalone option I kept getting the file not found error. It seems that I'm not properly passing the files to the docker images build. Could you check the current Dockerfile? I think that it is correct but I still get the said error.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 31, 2024

I'm still trying to understand the changes proposed in this PR...

Pretty much we want to make sure that our passed data_path and config_path parameters are used throughout EMHASS. Currently there are functions inside of EMHASS that have datapaths set to like: root + /data/filename. However, this may not always be the case if we set the datapath to something like /share.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Apr 1, 2024

pytest wise, just got the

    def test_main_publish_data(self):
        opt_res = main()
>       self.assertTrue(opt_res==None)

Issue now.

Setting the test to:

    def test_main_publish_data(self):
        opt_res = main()
        self.assertFalse(opt_res.empty)

Resolves that test. I'm just not sure why it was opt_res==None before hand?

@GeoDerp GeoDerp mentioned this pull request Apr 1, 2024
@davidusb-geek
Copy link
Owner

I'm still trying to understand the changes proposed in this PR...

Pretty much we want to make sure that our passed data_path and config_path parameters are used throughout EMHASS. Currently there are functions inside of EMHASS that have datapaths set to like: root + /data/filename. However, this may not always be the case if we set the datapath to something like /share.

Ok yes. I get it. It is a good idea to have a proper path management.

@davidusb-geek
Copy link
Owner

@GeoDerp Careful with those conflicts with forecast related files. I made some updates this week. You need to git rebase.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Apr 5, 2024

@GeoDerp Careful with those conflicts with forecast related files. I made some updates this week. You need to git rebase.

hahah, I just merged them.

May be good to look over, Just in case my tiredness didn't make a mistake.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Apr 5, 2024

def test_main_publish_data(self):
opt_res = main()
self.assertFalse(opt_res.empty)

I just changed this so all tests passed. Feel free to revert. Will be away for a few days. Feel free to have a look/adjust/implement in the mean time. Will defiantly recommend some tests to confirm everything is functional.

Copy link

codecov bot commented Apr 5, 2024

Codecov Report

Attention: Patch coverage is 60.25641% with 31 lines in your changes are missing coverage. Please review.

Project coverage is 72.88%. Comparing base (96e66b3) to head (5f62589).
Report is 1 commits behind head on master.

❗ Current head 5f62589 differs from pull request most recent head ce4048e. Consider uploading reports for the commit ce4048e to get more accurate results

Files Patch % Lines
src/emhass/forecast.py 60.71% 11 Missing ⚠️
src/emhass/command_line.py 75.00% 8 Missing ⚠️
src/emhass/web_server.py 0.00% 7 Missing ⚠️
src/emhass/retrieve_hass.py 40.00% 3 Missing ⚠️
src/emhass/utils.py 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #247      +/-   ##
==========================================
- Coverage   73.21%   72.88%   -0.34%     
==========================================
  Files           7        7              
  Lines        2035     2069      +34     
==========================================
+ Hits         1490     1508      +18     
- Misses        545      561      +16     

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

@davidusb-geek
Copy link
Owner

I just changed this so all tests passed. Feel free to revert. Will be away for a few days. Feel free to have a look/adjust/implement in the mean time. Will defiantly recommend some tests to confirm everything is functional.

But this is not ready to merge right?

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Apr 5, 2024

I just changed this so all tests passed. Feel free to revert. Will be away for a few days. Feel free to have a look/adjust/implement in the mean time. Will defiantly recommend some tests to confirm everything is functional.

But this is not ready to merge right?

I wouldn't be confident until I ran some more tests. But I also trust that you can do the same if you like to merge it sooner.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Apr 11, 2024

Having a look over the PR now, will likely be finished tomorrow night.

Edit: Tomorrow I'll double check, but it could be good to go.
Script wise, I was unable to test special_config_analysis.py properly. The rest seemed to work since my last test.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Apr 13, 2024

Will check over the latest changes tomorrow to confirm everything should be good to go.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Apr 14, 2024

@davidusb-geek , Alright I'm pretty happy with it, I may be missing some functions, the old path system is still used. But, from my testing, I believe everything is functional. Feel free to test yourself and see what you think.

The two things I'm unsure about is how well special_config_analysis.py and save_pvlib_module_inverter_database.py works as I'm unable to test it.

Also have a look at the two places I have reviewed to see if you agree to the warnings/errors.

@davidusb-geek
Copy link
Owner

@davidusb-geek , Alright I'm pretty happy with it, I may be missing some functions, the old path system is still used. But, from my testing, I believe everything is functional. Feel free to test yourself and see what you think.

The two things I'm unsure about is how well special_config_analysis.py and save_pvlib_module_inverter_database.py works as I'm unable to test it.

Ok it will be merged and tested, probably over the week-end. For those 2 files I can check them, but those are just auxiliary scripts files.

@davidusb-geek davidusb-geek merged commit 34def7d into davidusb-geek:master Apr 18, 2024
13 checks passed
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.

None yet

2 participants