Create abstract base class for inverters #183
Conversation
# Conflicts: # quartz_solar_forecast/inverters/enphase.py # quartz_solar_forecast/pydantic_models.py
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #183 +/- ##
===========================================
- Coverage 79.31% 58.33% -20.98%
===========================================
Files 18 23 +5
Lines 551 984 +433
===========================================
+ Hits 437 574 +137
- Misses 114 410 +296 ☔ View full report in Codecov by Sentry. |
|
Thanks @mduffin95, this is very neat, Really appreciate all this work youve done. @aryanbhosale would you like to review this? |
Yup sure, I'll have a look at this right now |
Hey @peterdudfield I've added some reviews , do you want me to review something again or is this fine? |
|
@aryanbhosale I can't see any comments. Have you submitted the review? |
Hey yes I've added the reviews, check the file changes tab probably it'll show up there |
|
@aryanbhosale any comments added should show up on this page. You'd also show up in the reviewers section if you'd submitted a review. Do the comments say "pending" next to them for you? |
|
@aryanbhosale this means you need to submit the review before I can see the comments. |
There was a problem hiding this comment.
Hey @mduffin95 ,thanks for this, I've put some reviews - mostly on separation of concerns (model classes from processing) and making it compatible with the existing api functionality
| @@ -144,61 +135,11 @@ def format_nwp_data(df: pd.DataFrame, nwp_source:str, site: PVSite): | |||
| ) | |||
| return data_xr | |||
|
|
|||
| def fetch_enphase_data() -> Optional[pd.DataFrame]: | |||
There was a problem hiding this comment.
Why have these fetch functions for different inverter brands been removed?
There was a problem hiding this comment.
The functionality has just been moved to the get_data method for each inverter.
Ah my bad, thank you for that(bad network on my side so didn't get submitted) |
There was a problem hiding this comment.
Have these been tested @mduffin95 - with the api and the dashboard?
|
I get these errors with enphase |
It's the same issue with all the inverters - not just enphase |
|
It looks like that's because the env file has multiple variables set up. I only tried it with variables for GivEnergy. If we want to allow that I should be able to tweak the settings. |
@mduffin95 error with enphase: GivEnergy plot with live kw might be inconsistent because of an extra/1000 - please check your code since the output should not be this far from the forecasted one for Solis it doesnt fetch any live data: (Error retrieving Solis data: Timeout error occurred) Solarman: this is the same as givenergy, please look out for the extra /1000 which is causing this inconsistency i think once these errors are solved this can be merged in |
@aryanbhosale have you checked this functionality on the Regarding this issue: I've pushed a chane that I think might fix this. If it doesn't work do you think you might be able to take a quick look to find the fix yourself? As I don't have enphase credentials it's difficult for me to do. |
What do you think could be the issue here? |
|
@mduffin95 everything works currently but with enphase its a little weird, even after i type the url, when i go back to my CLI, i see this: This shouldnt happen, if im using something in the API/dashboard context everything should be done on the dashboard side itself, and when im using the CLI context, it should show up in the CLI, there shouldnt be a mismatch bw the 2, the code for this is currently in the recently pushed main branch of this repo anyway if i paste the url in the cli after its prompted, it says this: We're using env vars to store the access/refresh token as of now when using the cli context, and conditionally checking if they exist, then dont go into the CLI context(in the current main branch in enphase.py) Would be great if you could fix this, everything else works perfectly fine now |
|
one more thing @mduffin95 , when No Inverter is chosen, the columns should have only power_kw like this: currently it has both power_kw and power_no_live_kw, and theyre both the same which doesnt make sense to have, it would be good if that change is made too before merging this in |
|
Great work here @mduffin95 ! It's fixed and works as expected. Thanks a lot for this. |
|
hey @mduffin95 , do you mind having a look at the current docs for the api, as well as the dashboard_2 docs, and see if your changes affect anything there? and update it accordingly, it would be great if we could get this done in the same relevant PR |
|
I've added a line to the docs about making sure new inverter models extend |
Thank you for this |
Can this be merged? |
If yourself and @peterdudfield are happy with it too then yes I would say go ahead, maybe squash and merge as it's quite a few commits |
I'm happy with it too, just waiting for @peterdudfield to give his comments over this before merging this in |
|
In fact I would say go ahead now, you worked on this most @aryanbhosale so your approval was the key thing, so feel free to merge @mduffin95! Thanks again |
Pull Request
Description
How Has This Been Tested?
Kind of difficult to test as I don't have access to all of these inverter types. However I have managed to get access to a GivEnergy system and I've managed to test this one out.
It would be good to mock out some API calls to various inverters so that they can be tested.
If your changes affect data processing, have you plotted any changes? i.e. have you done a quick sanity check?
Checklist: