Dynamic paths with emhass config dict, some mlforcaster error suppression

GeoDerp commented 3 months ago

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.

GeoDerp commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

davidusb-geek commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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?

davidusb-geek commented 3 months ago

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 commented 3 months ago

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

GeoDerp commented 3 months ago

@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 commented 3 months ago

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.

codecov[bot] commented 3 months ago

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.

:exclamation: 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 :warning:
src/emhass/command_line.py	75.00%	8 Missing :warning:
src/emhass/web_server.py	0.00%	7 Missing :warning:
src/emhass/retrieve_hass.py	40.00%	3 Missing :warning:
src/emhass/utils.py	60.00%	2 Missing :warning:

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

davidusb-geek commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

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

GeoDerp commented 2 months ago

@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 commented 2 months ago

@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 / emhass