-
-
Notifications
You must be signed in to change notification settings - Fork 59
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
Implementation of other sensors #134
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! A good improvement over the original one, just a few changes I think, then we can merge this in.
graph_weather/__init__.py
Outdated
@@ -1,4 +1,5 @@ | |||
"""Main import for the complete models""" | |||
|
|||
from .data.nnjai_wrapp import SensorDataset, collate_fn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should actually import this here, since we have nnja
as an optional dependency. I think to import this dataset, like in the tests, we should just do from graph_weather.data.nnja_ai import SensorDataset
for example. I also renamed this on the main branch, but could you update the nnjai_wrapp.py
to be named nnja_ai
as I think that is maybe more consistent naming.
graph_weather/data/nnjai_wrapp.py
Outdated
) | ||
|
||
|
||
class SensorDataset(Dataset): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class SensorDataset(Dataset): | |
class NNJADataset(Dataset): |
We might (probably will, as I am looking into adding other, non NNJA sensors) have other sensors that won't fit in this dataset. I think naming this just NNJADataset
then is more descriptive and easier to parse where this data is coming from.
Could you please tell me the status of any changes for this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few changes needed I think for updating the tests and removing an old file. But then I think it would be good to merge. If you can also resolve the merge conflicts, that would be great.
graph_weather/data/nnjai_wrapp.py
Outdated
@@ -0,0 +1,106 @@ | |||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove this file as its not used now?
tests/test_nnjai.py
Outdated
@@ -0,0 +1,166 @@ | |||
""" | |||
Tests for the nnjai_wrapp module in the graph_weather package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests for the nnjai_wrapp module in the graph_weather package. | |
Tests for the nnja_ai module in the graph_weather package. |
tests/test_nnjai.py
Outdated
time = datetime(2021, 1, 1, 0, 0) # Using datetime object instead of string | ||
primary_descriptors = ["OBS_TIMESTAMP", "LAT", "LON"] | ||
additional_variables = ["TMBR_00001", "TMBR_00002"] | ||
dataset = SensorDataset(dataset_name, time, primary_descriptors, additional_variables, sensor_type="AMSU") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these tests still pass? I think they need to be updated for the new names. Ideally, there should be tests for all the different sensor types as well in NNJADataset
. You should be able to do that with pytest.parameterize
I think.
Pull Request
Description
Added implementation for other tensor datasets and their unit tests for sensor data processing functionality.
This PR adds comprehensive unit tests for the sensor data processing module, specifically testing the
SensorDataset
class andcollate_fn
function. The tests cover various sensor types including AMSU-A, ATMS, MHS, IASI, and CrIS.Key changes:
test_sensor_dataset
to verify basic dataset functionalitytest_collate_function
to ensure proper data batchingtest_sensor_datasets
to test multiple sensor typesFixes #128
Test Results
Test output
Testing Details
Checklist:
The PR focuses on improving test coverage for the sensor data processing pipeline, ensuring reliable data handling across different sensor types while maintaining code quality standards.