MHKiT-Software / MHKiT-MATLAB

MHKiT-MATLAB provides the marine renewable energy (MRE) community tools for data processing, visualization, quality control, resource assessment, and device performance.
https://mhkit-software.github.io/MHKiT/
BSD 3-Clause "New" or "Revised" License
15 stars 23 forks source link

CDIP Data IO and Graphics (v2) [develop] #83

Closed Matthew-Boyd closed 2 years ago

Matthew-Boyd commented 2 years ago

This pull request replicates the CDIP functionality in MHKiT-Python, including:

The main functional difference from the Python implementation is that only the selected data is queried, not the entire netCDF file.

Closes #58

image

image

rpauly18 commented 2 years ago

@Matthew-Boyd can you give me permission to push to your repo?

rpauly18 commented 2 years ago

I am having issues with the cdip tests. All of the tests are failing with this error:

Error ID:
---------
'MATLAB:UndefinedFunction'
--------------
Error Details:
--------------
Unrecognized function or variable 'value'.

Error in cdip_request_parse_workflow (line 127)
                data.(type).(group_name).(name) = value;

Error in Wave_TestIOcdip/test_plot_compendium2 (line 127)
            data = cdip_request_parse_workflow( ...
rpauly18 commented 2 years ago

Additionally, are we expecting warnings for data not being found? Example below: Warning: Data name 'gpsLatitude' not found.

In cdip_request_parse_workflow (line 110) In Wave_TestIOcdip/test_request_parse_workflow_no_times (line 84) Warning: Data name 'gpsLongitude' not found. In cdip_request_parse_workflow (line 110) In Wave_TestIOcdip/test_request_parse_workflow_no_times (line 84)

Matthew-Boyd commented 2 years ago

I am having issues with the cdip tests. All of the tests are failing with this error:


Error ID:
---------
'MATLAB:UndefinedFunction'
--------------

I get the same errors if I don't have /mhkit/wave/IO/CDIP added to my MATLAB path. I assume we update the toolbox for handling new paths?

Matthew-Boyd commented 2 years ago

@Matthew-Boyd can you give me permission to push to your repo?

This PR branch is on the main repo, not my fork.

rpauly18 commented 2 years ago

I am having issues with the cdip tests. All of the tests are failing with this error:

Error ID:
---------
'MATLAB:UndefinedFunction'
--------------

I get the same errors if I don't have /mhkit/wave/IO/CDIP added to my MATLAB path. I assume we update the toolbox for handling new paths?

I did add the functions to my path. This error is specific to something inside the function where "value" is not being defined.

Matthew-Boyd commented 2 years ago

Additionally, are we expecting warnings for data not being found? Example below: Warning: Data name 'gpsLatitude' not found.

I added warnings for non-existent parameters in a new commit, 421eeba82009426c54b4d58160b3964743183f39

rpauly18 commented 2 years ago

@Matthew-Boyd the changes you made now make it so the code runs, but the only thing that gets passed back from cdop_request_parse_workflow is the metadata - no actual data.

rpauly18 commented 2 years ago

Also, not entirely sure why the CI tests are not being triggered for this PR. Is it because we are trying to merge from a branch and not another fork?

It is looking like this is either a Mac/Windows issue or a Matlab version issue. The best way to narrow that down will be to get the CI tests up and running on this PR

Matthew-Boyd commented 2 years ago

@Matthew-Boyd the changes you made now make it so the code runs, but the only thing that gets passed back from cdop_request_parse_workflow is the metadata - no actual data.

I'm guessing it's a similar kind of access failure, but that the new lower-level NetCDF code I added is causing a different exception to be thrown than the one our code is looking for on line 110, thus it just continues on if the value can't (normally) be read. Maybe see what exceptions (identifier and message) are being thrown on line 110? Or happy to do another working session at your convenience.

There is a known memory leak in the NetCDF library, pre-R2020b Update 3, (link) which may be to blame here. I was hoping to get around it with this recent commit that replaced ncread() with lower-level functions, but that may not have worked.

Matthew-Boyd commented 2 years ago

Also, not entirely sure why the CI tests are not being triggered for this PR.

@rpauly18 I merged the latest commits on develop into this branch and that got the tests running. Can you try running test "test_request_usgs_data_instant" locally on your macOS machine? It passes on my Windows machine and there's not enough details as to why it's failing on some of the runners. (Maybe something to expound on in our Action.)

rpauly18 commented 2 years ago

The tests that are failing happen periodically, even on Github actions. When it fails, we just need to restart the tests. It happens because we are making too many requests to the USGS server I believe.

Interesting that the clip tests are passing though - means the issue is somewhere in my own system

Matthew-Boyd commented 2 years ago

Please do still record the exception identifier and message you are getting so we can properly handle that for others who may have your same issue.

On Mon, Feb 28, 2022 at 9:55 AM rpauly18 @.***> wrote:

The tests that are failing happen periodically, even on Github actions. When it fails, we just need to restart the tests. It happens because we are making too many requests to the USGS server I believe.

Interesting that the clip tests are passing though - means the issue is somewhere in my own system

— Reply to this email directly, view it on GitHub https://github.com/MHKiT-Software/MHKiT-MATLAB/pull/83#issuecomment-1054459500, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHICFB5HG4INVXUOUGQVKU3U5OSIJANCNFSM5OMHPUIQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

rpauly18 commented 2 years ago

This is the exception that is being thrown: Warning: Data name 'waveTp' not found.

In cdip_request_parse_workflow (line 114) In Wave_TestIOcdip/test_plot_compendium2 (line 127)

The underlying errors are still the same as what was being thrown when we had out working session

Matthew-Boyd commented 2 years ago

This is the exception that is being thrown: Warning: Data name 'waveTp' not found.

In cdip_request_parse_workflow (line 114) In Wave_TestIOcdip/test_plot_compendium2 (line 127)

The underlying errors are still the same as what was being thrown when we had out working session

Are you seeing the warning "Access failure to NetCDF file..." ? If not, the exception is likely different and thus not being caught by line 110. The "Data name not found" warning needs to be separate to alert on actually missing or mistyped variables.

If the Python implementation of the CDIP IO works fine for you, then I'm guessing the issues is just the older MATLAB version. Once we can properly catch the exception we can add a note in the warning to that effect.

rpauly18 commented 2 years ago

@Matthew-Boyd I suspect you are right about the Matlab version. I am pestering IT to get my version updated but they are not cooperating. I want to confirm so we can update the Matlab version requirements

rpauly18 commented 2 years ago

I am going to merge this since we are confident that this works. We still need to resolve the Github actions issue