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.

Sign up for GitHub

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

Jump to bottom

NRCC Trained Canadian Airspace Models #49

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

NRCC Trained Canadian Airspace Models #49

wants to merge 14 commits into from

Conversation

spydoctor
Copy link

Add support for the Canadian Airspace Models

@spydoctor
Add files via upload
d9fde38 
Add support for the Canadian Airspace Models
@spydoctor
Copy link
Author

This pull request is to enable compliancy with the Canadian airspace models. Several MIT LL files were modified, and an extra function was created to read Canadian data format.

Additional requirements:

  1. Download the latest version of Canadian models from https://github.com/nrc-cnrc/Canadian-Airspace-Models
  2. Set up system variable pointing to Matlab_Frequency_Table folder in Canadian-Airspace-Models repository

aweinert-MIT
aweinert-MIT requested changes Jul 18, 2023
View reviewed changes
Copy link
Member

@aweinert-MIT aweinert-MIT left a comment

Choose a reason for hiding this comment

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

Initial review focused on addressing duplicate files and streamlining Canadian contributions

em_read_CAN.m Outdated
@@ -0,0 +1,69 @@
% Copyright (c) 2023 Carleton University and National Research Council Canada
Copy link
Member

Choose a reason for hiding this comment

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

Only em_read_CAN has the Carleton and NRCC copyright. Is this copyright required on all files created or modified by Carleton and NRCC?

Copy link
Author

@spydoctor spydoctor Jul 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I will add copyright statement to other modified files

EncounterModel.m Outdated
@@ -0,0 +1,357 @@
classdef EncounterModel < handle
Copy link
Member

Choose a reason for hiding this comment

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

Is this file a copy of code\matlab\@EncounterModel\EncounterModel? If a copy, please remove this duplicate. If this code was modified, can you please update of the existing file?

Copy link
Author

Choose a reason for hiding this comment

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

This code was modified.

Copy link
Author

@spydoctor spydoctor Jul 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I understood what you meant by duplicates - there were extra files committed to the main folder. This was corrected

RUN_uncor.m Outdated
%% Inputs
file_name='uncor_1200only_fwse_v1p2.txt';

if contains (file_name, '.txt')
Copy link
Member

Choose a reason for hiding this comment

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

I recommend creating a new file titled RUN_uncor_CA in the code\matlab directory and streamline this new file to load only the new Canadian model.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, will do!

RUN_uncor.m Outdated
% Geodetic track maintaining at least 500 feet laterally from a point obstacle
out_results_geo500 = mdl.track(n_samples, sample_time, 'initialSeed', init_seed, 'coordSys', 'geodetic', ...
'lat0_deg', lat0_deg, 'lon0_deg', lon0_deg, ...
'dofMaxRange_ft', 500, 'isPlot', true);
Copy link
Member

Choose a reason for hiding this comment

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

dofMaxRange_ft is for when using the FAA digital obstacle file (DOF). The DOF has limited coverage of the Pacific, the Caribbean, Canada, and Mexico. Recommend adding an inline comment about this or in a future update, provide support for more Canadian obstacles.

Copy link
Author

Choose a reason for hiding this comment

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

I removed this piece of code from RUN_uncor_CA.m, since we currently do not support this.

RUN_uncor.m Outdated
% lat0_deg = 44.25889; lon0_deg = -71.31887; % Lake of the Clouds, White Mountains, NH
% lat0_deg = 40.01031; lon0_deg = -105.22097; % Flatirons Golf Course, Boulder, CO
% lat0_deg = 46.96983; lon0_deg = -101.54661; % Bison Wind Project, ND
lat0_deg = 42.29959; lon0_deg = -71.22220; % Exit 35C on I95, Massachusetts
Copy link
Member

Choose a reason for hiding this comment

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

Please update lat0_deg and lon0_deg to be a location in Canada

Copy link
Author

Choose a reason for hiding this comment

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

I removed this piece of code from RUN_uncor_CA.m, since we currently do not support this.

RUN_uncor.m Outdated
% update or comment out accordingly. Also comment out if you don't want to
% define a start distribution.
%To use Canadian models, please comment those out
start{1} = 1; % Geographic domain - CONUS (G = 1)
Copy link
Member

Choose a reason for hiding this comment

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

Remove start for Canadian specific file

Copy link
Author

Choose a reason for hiding this comment

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

Done!

README.md
@@ -1,288 +1,49 @@
# Bayesian Network Encounter Models
Copy link
Member

Choose a reason for hiding this comment

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

The main README should include the intro material

Copy link
Author

@spydoctor spydoctor Jul 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure how the main Readme got to this condition!! And there were duplicates all over the main folder. Perhaps, there was a mistake on my end upon the creation of this pull request.

UncorEncounterModel.m Outdated
@@ -0,0 +1,535 @@
classdef UncorEncounterModel < EncounterModel
Copy link
Member

Choose a reason for hiding this comment

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

Similar comments as EncounterModel. If this is a copy, use the existing version. If this was modified, please streamline

Copy link
Author

Choose a reason for hiding this comment

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

This file was modified

Copy link
Author

Choose a reason for hiding this comment

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

Duplicate removed

@aweinert-MIT aweinert-MIT added documentation Improvements or additions to documentation enhancement New feature or request question Further information is requested labels Jul 18, 2023
@aweinert-MIT aweinert-MIT changed the title Add files via upload NRCC Trained Canadian Airspace Models Jul 18, 2023
aweinert-MIT
aweinert-MIT reviewed Jul 18, 2023
View reviewed changes
code/matlab/@EncounterModel_CA/EncounterModel_CA.m Outdated
@@ -0,0 +1,354 @@
classdef EncounterModel_CA < handle
Copy link
Member

Choose a reason for hiding this comment

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

Can this class inherit from EncounterModel or UncorEncounterModel? I really want to minimize duplicate code

Copy link
Author

Choose a reason for hiding this comment

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

Just fixed. Please let me know if you have any other requests related to reducing redundancy ;)

aweinert-MIT
aweinert-MIT reviewed Jul 18, 2023
View reviewed changes
code/matlab/@UncorEncounterModel_CA/getDynamicLimits.m Outdated
@@ -0,0 +1,130 @@
function dynamiclimits = getDynamicLimits(self, initial, results, idx_G, idx_A, idx_L, idx_V, idx_DH, is_discretized)
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a duplicate file?

Copy link
Author

Choose a reason for hiding this comment

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

Just removed!

spydoctor added 3 commits July 18, 2023 16:56
@spydoctor
Addressed latest request to add inheritance
a14a823
@spydoctor
Addressing the issue with inheritance
1812bf4 
Previous commit I uploaded old version accidentally ; those modes should be correct
@spydoctor
Delete getDynamicLimits.m
8609cbb
@aweinert-MIT
Copy link
Member

aweinert-MIT commented Aug 1, 2023 •
edited
Loading

@spydoctor , In commit 5651666 I converted the helicopter .mat parameter file to an ASCII format and prototyped reading the ASCII file using the existing UncorEncounterModel class. Please confirm this was implemented correctly. If yes, I think we can update EncounterModel to read the .mat file OR provide a function to write the ASCII file. I think the latter is the more sustainable option.

@spydoctor
Copy link
Author

@aweinert-MIT I just tried to run the updated codes from your new commit, and I ran into several issues executing both run_uncor.m and run_uncor.ca (screenshots attached). Therefore, I could not verify, unfortunately, whether the conversion worked.
FYI, when I checked previous commits, the codes executed without any issues. I checked for both Matlab 2019 and Matlab 2022.

Run_uncor_ca_error
Run_uncor_error

@aweinert-MIT
Copy link
Member

I'll investigate and check everything was committed

@aweinert-MIT
Copy link
Member

@spydoctor Please confirm the system environment variable AEM_DIR_BAYES is set

@spydoctor
Copy link
Author

I confirm. If it wasn't set, previous versions wouldn't have worked.
image

@spydoctor
Copy link
Author

I re-installed everything, and linked everything again.
Now, I get a new error:

image

@spydoctor
Copy link
Author

This is very strange, because when I revert to a previous commit, everything works!

@aweinert-MIT
Copy link
Member

aweinert-MIT commented Aug 3, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aweinert-MIT aweinert-MIT aweinert-MIT requested changes

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation enhancement New feature or request question Further information is requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@spydoctor @aweinert-MIT