Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fds 1797 input graph api #1481
base: develop
Are you sure you want to change the base?
Fds 1797 input graph api #1481
Changes from 96 commits
2527224
70bfc19
5b41aa5
76a5567
670b5c7
a87d2fe
07dbd70
421c59a
2f5abf1
119cf40
c6c5cb8
dd8b2cd
6ec81db
bbe6c98
b99d9eb
1189107
86f7af8
d3c3769
215cc86
242c885
a1cfcce
1c67b4a
a986b39
0390b69
37bbf74
441b865
fe4fffb
8c64981
f20f88b
d56c965
4596560
b5dd8e4
c8da900
601b5d0
c6d01de
1f57cf1
99e860b
d89dc10
c9786dc
38b0690
77979c7
9907b4e
481ebd3
f63231a
f3fdc41
032771f
bf0e9ba
bc59218
5973a75
bf1a449
69bb182
d93aa9d
aef4f62
08b2ae5
a088d63
68672c3
de07c55
77f5ec4
5934ed5
51313bd
e4e1368
9a78a18
05b4fe0
0964501
697cc3e
774dc91
bb7c450
8f5bc1a
35c13dd
f8c14d4
13c9ffc
9205d6d
172e1b1
d4d3a30
730b227
795e263
0ddb552
537af6f
dcb2f65
bb4ee3a
5504586
b88ee86
2bf450b
7adaf44
c2a06f6
1e5bd7d
c849f92
8041b07
0870fa1
6802753
23b7e1d
fa5e308
3dd6659
f28a0d7
afbe448
e4d0b7c
a7d7366
ee9cb9f
cec7aa6
92b66fd
b52bfdc
8a64c39
5bd3fc0
9c2edde
58c1429
487e31d
e13f693
3d5da40
a331725
3ed1177
b6728d3
b7460ea
53e639b
498851b
58aef40
03f7497
bc64305
2a7ea55
275fbae
89cd4ce
0f4091a
3197abf
abd1630
445aad5
0c4fad7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is already loaded on line 25
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.
so the logic from 1685 to 1703 can be further improved. Essentially you want to cover the following cases:
so instead of starting with
if graph_data_mode is None
, it will be easier if you start with if graph_data_modeland also, please consider wrapping this part in its own function... that way testing can be easier
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 this comment meant to be addressed in this PR? If not, can a new ticket be created so that we don't lose track?
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.
Yep, also this is formatted like a DOC string with triple quotes instead of starting with a hash like a comment.
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.
Generally things like this or
# TODO
are fine if they are just temporary, but should be removed before submitting a PR. At thet point they should be fixed, or have a Jira issue created as Lingling said.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 think you copied this from
output_jsonld
but forgot to change?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 think the CLI part still needs some more work and design thinking. When I am reading this, I think
--output_jsonld
,output_type
, andoutput_path
are very confusing to the users.Can we simplify this by combining --output_jsonld and --output_path into a single
--output_path
option or just--output
?I am seeing:
what is the purpose of adding output_path parameter if it is just using the value from output_jsonld?
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.
so many
if else
are added here to handle different cases ofoutput_path
andoutput_jsonld
.. it is hard for the user to know which one take precedence. It is better in my opinion to simplify the parameter and clean up the logic here.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.
Here, I think the read_pickle function assumes that the pickle file can be correctly loaded. But what if it doesn't? Could you raise a meaningful error message here?
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.
Can test for read_pickle also be added?
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.
Will self.graph_data_model always be assigned a value? The logic here is kind of confusion, and it's nto clear it always will be.