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

recover new south wales #1107

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

recover new south wales #1107

wants to merge 4 commits into from

Conversation

majesticio
Copy link
Contributor

This is a draft. it appears to be returning valid measurements but is not finishing:

info: 0 new measurements found for "Australia - New South Wales" in 1.553s
info: Australia - New South Wales - 1 occurrences of Adapter error (au.json/nsw/'Australia - New South Wales'): Cannot read properties of undefined (reading 'readable')

@majesticio majesticio requested a review from caparker April 12, 2024 23:40
  Needs work on the parameters and units
@caparker
Copy link
Collaborator

I got it working and did some cleanup but there is still work to do on the parameters + units.

A few notes that I wanted to share
You were catching errors in both the fetchStations and the fetchMeasurements methods and then not rethrowing them. Each of those methods must run successfully in order for the how adapter to work. Given that and based how you designed this I would have expected either

  1. No error catching in either of those methods. The try/catch in the fetchData method would catch it and we would just have to hope that the message returned was indicative of what method it happened in.
  2. Catch the error in each method but then rethrow it with more detail about where it happened.

Also the repetitive use of find can be fixed by objectifying the array and keying it on Site_Id.

Make sure that all the units are units we allow and if they are not we will need to fix them.

@russbiggs
Copy link
Member

This needs quite a bit more work in the documentation doc before proceeding with development. Once documentation is more thorough we can proceed

@majesticio
Copy link
Contributor Author

I corrected the use of the timezone ('Australia/Sydney'). The source data date times are all in local time. I made note of it in our documents for future reference

Copy link
Collaborator

@caparker caparker left a comment

Choose a reason for hiding this comment

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

I am still seeing the parameter issues. Here is what I get

debug: Source Australia - New South Wales is "finished"
debug: Fetch results for Australia - New South Wales
debug: Started Australia - New South Wales - 1713275870399
info: 273 new measurements found for "Australia - New South Wales" in 1.932s
info: Australia - New South Wales - 58 occurrences of instance.parameter is not one of enum values: pm25,pm10,no2,so2,o3,co,bc,no,nox,pm1
info: Australia - New South Wales - 58 occurrences of instance.unit is not one of enum values: µg/m³,ppm,ppb
info: Australia - New South Wales - 27 occurrences of instance.value is not of a type(s) number
info: Finished with 1 successes and 0 failures in 2.018 seconds
debug: 93 locations of 7 parameters from 2024-04-16T13:00:00.000Z - 2024-04-16T13:00:00.000Z | 
Parameters for Australia - New South Wales {
"co":{"count":19,"errors":0,"max":0.317628,"min":-0.152889,"nulls":0},
"no":{"count":43,"errors":0,"max":0.07599178000000001,"min":-0.00096442,"nulls":0},
"no2":{"count":43,"errors":0,"max":0.0318994,"min":0,"nulls":0},
"o3":{"count":42,"errors":0,"max":0.03167671,"min":-0.001845,"nulls":0},
"pm10":{"count":55,"errors":0,"max":120.9,"min":-2.636,"nulls":0},
"pm25":{"count":35,"errors":0,"max":33,"min":0,"nulls":0},
"so2":{"count":36,"errors":0,"max":0.00209404,"min":-0.00064265,"nulls":0}
}

Looks like we are only allowing 7 of the 8 parameters through the parameter check. Look to see if we need to remove one from the adapters parameter list or add it to our accepted parameters.
Also, we are getting null value errors. To be consistent with other sources I think we should stop throwing those out and instead turn them into 9999s.

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