-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add support to SeaExplorer gliders #3
Conversation
Thanks for the PR. Let's go with the revision. |
error('glider_toolbox:posixtime:MissingMexFile', ... | ||
'Missing required mex file.'); | ||
else % -FC | ||
t = (now()-datenum(1970,1,1))*86400; |
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.
These changes should be reset (git reset v1.1.1 m/mex_tools/posixtime.m
).
The required mex file can be build with setupMexPosixtime
,
(it simply exposes the time
function from the C standard library).
Even though the conversion here is correct, and you can use it
if you do not want to compile the mex file, it should not be part of 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.
OK. I did not know how needed to compile in C first. Now it works.
Please feel free to commit and push your fixes to your forked repository when they are ready. |
preprocessing_options.depth_list(1).depth = 'GPCTD_PRESSURE'; | ||
preprocessing_options.depth_list(2).depth = 'SBD_PRESSURE'; | ||
%preprocessing_options.depth_list(3).depth = 'NAV_DEPTH'; | ||
|
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.
Is there any reason to not use the navigation depth Depth
here?
Note that, in addition to this depth, you can also enable the depth derivation from CTD pressure,
(which derives the depth using the pressure and the latitude).
You will end up with two variables: depth
and depth_ctd
.
They might or might not coincide, depending on how the glider works and the sampling configuration options.
Does the SeaExplorer have a dedicated pressure sensor for navigation or uses the one from the CTD?
Can/Do the navigation depth have a different sampling configuration than the CTD pressure?
Sometimes it might useful to have both of them, for example if the CTD sampling is disabled
while other sensors are enabled.
Of course this is just the default configuration, users can adjust it to their need in their runtime environment.
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.
Indeed, I was a bit messed with the depth
and depth_ctd
variables.
There are 2 pressure sensors on SeaExplorer. One for Navigation that return Depth
and NAV_DEPTH
. They are the same except one is sub-sampled and found in navigation files (Depth
) while the other is found in the higher sampling rate science files. These are not always well calibrated
There is also the pressure sensor of the CTD which is the only one reliable here: SBD_PRESSURE
or GPCTD_PRESSURE
.
Should I keep both variables under depth
and depth_ctd
?
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.
As an addition to this comment: does the pressure converted to depth using hydrostatic relationship somewhere? or using depth_ctd is actually pressure?
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.
In that case I would suggest to keep the navigation depth
(ordered list of preferred sources: NAV_DEPTH
,Depth
) as depth
,
and allow the depth from pressure derivation, resulting in the depth_ctd
variable.
The rationale is that the GPCTD sampling might be disabled in some stage of a deployment while other sensors are left on, and there will be no vertical coordinate for those readings
(or a very poorly estimated one by interpolation of the previous and next
pressure measurements, which could be too far away),
Actually this is a common situation if the CTD is configured to sample only on downcasts.
Having both depth sequences in the final processed data allows the end user
to choose the best vertical reference coordinate for the data he wants to consume.
The depth_ctd
is derived from pressure and latitude using CSIRO's sw_dpth
in processGliderData.m
.
I merged your PR into the
|
Wow, awesome, many thanks! |
With Then you can Alternatively you can pull the changes into your |
I tested the new version, it looks like it works very well on the data. Here are the answers to your questions:
Other points to discuss about the merge:
Please instruct me if I should go forward with the changes or if you prefer handling them. |
|
Answers in order:
|
Regarding the payload configuration, I was talking about the name of the attribute I will update code and references to .dat files to use .pld instead, and the name patterns
Regarding the real time file access, I see. If that machine at the manufacturer can be accessed |
Ok sorry for the attribute. Better indeed! I would prefer option 2 for pld name. But you should read Better wait then. It is not accessible via FTP at the moment. |
Change the merging algorithm of `sxmerge` so that data entries from gli and dat datasets with the same timestamp are properly merged, preserving all the values instead of discarding those from the gli dataset. Even though no variable duplication is expected between gli and dat data, guard against future issues by checking the consistency of entries of duplicated variables at the same time insant, if any. Report detailed information about the inconsistencies in the error message. Also update `sxcat` and `sx2mat` to match the coding style and the error messages, and pass the proper time options to the low level functions in `loadSeaExplorerData`.
According to the manufacturer, the `.dat` suffix of the SeaExplorer science data files will be changed for `.pld` (payload). Update code and documentation to reflect the change in the nomenclature.
The nomenclature of SeaExplorer files is updated now in the The only thing left is the real time support. In order to close the PR, if we can not figure out |
I know how the realtime data will be retrieved, but it is not implemented yet (server not installed). But the idea is that realtime data comming from IRIDIUM will be synchronized on a linux server accessible via ssh. But for the moment it is also okay to me if an error is raised if an attempt is made. Concerning the PR, there is one little change I would like to add in I would like to implement it on the latest version, but for some reason I cannot checkout the last commits:
Here commit 51b085f... is not the most recent one. Am I doing something wrong? |
That approach for RT data retrieval seems sound to me. And for the the last changes: yes, you are doing something wrong: What you want to do is to fetch the last changes in the branch |
Yes, I remember your previous comment (and I tried to read the doc.) but my problem is fairly simple: I don't know with what I should replace I tried
|
Then the reference is this: https://git-scm.com/book/en/v2/Git-Basics-Working-with-Remotes |
Use the new variable names from the manufacturer to distinguish both Minifluo-UV sensors. Output in NetCDF files are now expressed in excitation/emission couple, both for detection and monitoring channels: 'fluorescence_255_360' 'fluorescence_260_315' 'fluorescence_270_340' 'fluorescence_270_376' 'fluorescence_monitoring_255_360' 'fluorescence_monitoring_260_315' 'fluorescence_monitoring_270_340' 'fluorescence_monitoring_270_376'
See above a commit where variables names for Minifluo were updated. This version of the toolbox works fine on my SeaExplorer data. For my side, I do not expect to be able to perform real-time processing of the data before few months. So I let you decide whether or not you should close the PR and open a new one later. |
No problem with the new changes, I'll merge them, add the missing parts |
Integrate support for SeaExplorer gliders, with minimal adaptions: - Move oxygen frequency option from CTD to oxygen sensor in data preprocessing, and adapt the default preprocessing configuration. - Insignificant cosmetic style fixes. Close: #3
Well, I merged the PR in the |
I think I have found an annoying mistake in implementation of the SeaExplorer Case concerning the geolocation of profiles. Lat/Lon variables written in the raw files are the last GPS coordinates recorded. In case of multiple yos between surfacing, this means that all yos will be stamped with the same lat/lon. This caused the toolbox to impose the same location to different profiles that are clearly not at the same position. Before digging into the code to correct this, what would be the most clever way to do it in the philosophy of the toolbox? |
If the glider records the position coordinates only after the gps fixes at surface, If the glider is repeating the last GPS fix when subsurfacing, If, however, you need to handle the later case, you need a way to distinguish
|
Here I re-copy a discussion we had in private about this topic:
|
I also made this test to remove repeated data:
Which works okay for removing repetitions within the same profile, but cannot remove repetitions when they are seaparated by invalid values. The result is still that some different casts are attributed to the same geographic location. The (sloppy) workaround I found is the following:
Where
This seems to correct the problem. |
Let me add here for reference some of the final points in the private discussion:
From the above observations, and to separate concerns and avoid code repetitions, function valid = sxgoodfix(latitude, longitude, state)
valid = (state == 116) ...
& ~isnan(latitude) & ~isnan(longitude) ...
& ~(latitude == 0 & longitude == 0);
valid_indices = find(valid);
valid(valid_indices(2:end)) = ...
diff(latitude(valid)) ~= 0 & diff(longitude(valid)) ~= 0;
end The configuration to perform that good fix filtering would be something like this: preprocessing_options.position_list(1).longitude = 'NAV_LONGITUDE';
preprocessing_options.position_list(1).latitude = 'NAV_LATITUDE';
preprocessing_options.position_list(1).position_status = 'NAV_RESOURCE';
preprocessing_options.position_list(1).position_good = @sxgoodfix;
preprocessing_options.position_list(1).conversion = @nmea2deg;
preprocessing_options.position_list(2).longitude = 'Lon';
preprocessing_options.position_list(2).latitude = 'Lat';
preprocessing_options.position_list(2).position_status = 'NavState';
preprocessing_options.position_list(2).position_good = @sxgoodfix;
preprocessing_options.position_list(2).conversion = @nmea2deg; I am not sure whether that should be in the default configuration or not. Trials and opinions welcome. |
I confirm that the last proposition works on several deployments that I have: problems encountered in the earlier version where of multiple casts were associated to the same geographical location are gone. I suggest to keep as default until a new firmware is implemented in the SeaExplorer. Do you prefer implementing the changes yourself (e.g. writing header for |
It is already in develop (091b906). |
Please find here a version of the toolbox that now deals with SeaExplorer gldier data. It is the first time I do a pull request hopefully I am doing it okay.
The toolbox works at least on my local machine, but I had to modify 2 existing scripts that may affect the behavior with other gliders. Please verify this in priority:
posixtime
(L.55)gridGliderData
(L.320)Otherwise, all the changes I have made should not impact other gliders processing. These changes are tagged with a '-FC' comment. In order to test the toolbox, I have created the script
main_glider_data_processing_dtsx