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

adding a new typesystem: mapstringmapstringdouble #1490

Merged
merged 15 commits into from
Sep 14, 2019
Merged

adding a new typesystem: mapstringmapstringdouble #1490

merged 15 commits into from
Sep 14, 2019

Conversation

gwenchee
Copy link
Contributor

In this PR, i add a mapstringmapstringdouble type system to cyclus for future use in cyder and d3ploy

@gwenchee gwenchee self-assigned this Nov 19, 2018
@FlanFlanagan
Copy link
Member

Looks good Gwen. Is it usable from the Python bindings ?

@gwenchee
Copy link
Contributor Author

I’ve only tried it with C++ so far. Will try to see if it works with python

bam241
bam241 reviewed May 15, 2019
View reviewed changes
src/query_backend.h Outdated
VL_MAP_STRING_VL_MAP_VL_STRING_DOUBLE, // ["std::map<std::string, std::map<std::string, int>>", 3, ["HDF5", "SQLite"], ["VL_MAP", "STRING", ["VL_MAP", "VL_STRING", "INT"]], true]
VL_MAP_VL_STRING_MAP_VL_STRING_DOUBLE, // ["std::map<std::string, std::map<std::string, int>>", 3, ["HDF5", "SQLite"], ["VL_MAP", "VL_STRING", ["MAP", "VL_STRING", "INT"]], true]
VL_MAP_VL_STRING_VL_MAP_STRING_DOUBLE, // ["std::map<std::string, std::map<std::string, int>>", 3, ["HDF5", "SQLite"], ["VL_MAP", "VL_STRING", ["VL_MAP", "STRING", "INT"]], true]
VL_MAP_VL_STRING_VL_MAP_VL_STRING_DOUBLE, // ["std::map<std::string, std::map<std::string, int>>", 3, ["HDF5", "SQLite"], ["VL_MAP", "VL_STRING", ["VL_MAP", "VL_STRING", "INT"]], true]
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe comments are correct here.

Copy link
Member

Choose a reason for hiding this comment

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

int need to be double and I believe the 3 needs to be a 4 (4 variable length types in map<string,map<string,double>>)

Copy link
Member

Choose a reason for hiding this comment

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

@gwenchee : I think I agree with Baptiste. Can you contemplate and respond?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry! I must have missed your ping @bam241, I will make appropriate changes to the comments now.

@bam241
Copy link
Member

bam241 commented May 15, 2019 •
edited

@gwenchee I tried to draft a doc in the issue of the repo see #1508
I think you need to fix the comment in the query_backend file and add an Update function corresponding to your type in the Sha1 class (at end of the query_backend.h)

@bam241 bam241 self-requested a review May 16, 2019 19:16
katyhuff
katyhuff requested changes May 31, 2019
View reviewed changes
Copy link
Member

@katyhuff katyhuff left a comment

Choose a reason for hiding this comment

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

minor comments, one associated with the comment from @bam241 .

src/query_backend.h Outdated
VL_MAP_STRING_VL_MAP_VL_STRING_DOUBLE, // ["std::map<std::string, std::map<std::string, int>>", 3, ["HDF5", "SQLite"], ["VL_MAP", "STRING", ["VL_MAP", "VL_STRING", "INT"]], true]
VL_MAP_VL_STRING_MAP_VL_STRING_DOUBLE, // ["std::map<std::string, std::map<std::string, int>>", 3, ["HDF5", "SQLite"], ["VL_MAP", "VL_STRING", ["MAP", "VL_STRING", "INT"]], true]
VL_MAP_VL_STRING_VL_MAP_STRING_DOUBLE, // ["std::map<std::string, std::map<std::string, int>>", 3, ["HDF5", "SQLite"], ["VL_MAP", "VL_STRING", ["VL_MAP", "STRING", "INT"]], true]
VL_MAP_VL_STRING_VL_MAP_VL_STRING_DOUBLE, // ["std::map<std::string, std::map<std::string, int>>", 3, ["HDF5", "SQLite"], ["VL_MAP", "VL_STRING", ["VL_MAP", "VL_STRING", "INT"]], true]
Copy link
Member

Choose a reason for hiding this comment

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

@gwenchee : I think I agree with Baptiste. Can you contemplate and respond?

src/query_backend.h Outdated Show resolved Hide resolved
gwenchee and others added 3 commits May 31, 2019 17:04
@gwenchee gwenchee requested a review from katyhuff May 31, 2019 22:12
@gwenchee
Copy link
Contributor Author

I have updated the PR to include your suggested changes @bam241 :)

@gwenchee gwenchee mentioned this pull request May 31, 2019
Open
katyhuff
katyhuff requested changes May 31, 2019
View reviewed changes
src/query_backend.h Show resolved Hide resolved
bam241
bam241 requested changes Aug 2, 2019
View reviewed changes
Copy link
Member

@bam241 bam241 left a comment

Choose a reason for hiding this comment

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

One minor change and this should be good to go.
the test should be fine, the error in cymetric have been fixed since the last run....

src/query_backend.h Outdated Show resolved Hide resolved
src/query_backend.h Show resolved Hide resolved
bam241
bam241 requested changes Sep 10, 2019
View reviewed changes
Copy link
Member

@bam241 bam241 left a comment

Choose a reason for hiding this comment

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

@gwenchee could you add a news file to this PR ?

@gonuke
Copy link
Member

gonuke commented Sep 10, 2019

@bam241 how does this pass the news test?

@bam241
Copy link
Member

bam241 commented Sep 10, 2019

Don't know :(
Not supposed to...

@gwenchee
Copy link
Contributor Author

I've added the news file @bam241 @gonuke

bam241
bam241 approved these changes Sep 10, 2019
View reviewed changes
Copy link
Member

@bam241 bam241 left a comment

Choose a reason for hiding this comment

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

LGTM!

gonuke
gonuke approved these changes Sep 12, 2019
View reviewed changes
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Thanks @gwenchee. Happy to merge once @katyhuff confirms that her issues are resolved

bam241
bam241 requested changes Sep 13, 2019
View reviewed changes
Copy link
Member

@bam241 bam241 left a comment

Choose a reason for hiding this comment

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

sorry I messed it before...

newtypesystem Outdated Show resolved Hide resolved
bam241
bam241 approved these changes Sep 13, 2019
View reviewed changes
Copy link
Member

@bam241 bam241 left a comment

Choose a reason for hiding this comment

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

thx !

@gonuke
Copy link
Member

gonuke commented Sep 14, 2019

It looks like all of @katyhuff comments are resolved, so I'll merge.

Thanks for this addition @gwenchee

@gonuke gonuke merged commit 0e1a5f0 into cyclus:master Sep 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gonuke gonuke gonuke approved these changes

@bam241 bam241 bam241 approved these changes

@FlanFlanagan FlanFlanagan Awaiting requested review from FlanFlanagan

@jbae11 jbae11 Awaiting requested review from jbae11

@gtw2 gtw2 Awaiting requested review from gtw2

@katyhuff katyhuff Awaiting requested review from katyhuff

Assignees

@gwenchee gwenchee

Labels
Comp: Core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@gwenchee @FlanFlanagan @bam241 @gonuke @katyhuff