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.

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

Data_Engine: Adding create methods and first query methods for Table class #1289

Merged
merged 14 commits into from
Nov 14, 2019

Conversation

IsakNaslundBh
Copy link
Contributor

@IsakNaslundBh IsakNaslundBh commented Oct 29, 2019

NOTE: Depends on

BHoM/BHoM#595

Issues addressed by this PR

Closes #1285

Adding create methods and first Query methods for Tables.

Please note that the DataTable can not be serialised to Bson by default. I have code ready for this, but will need to be aligned with the work done by @adecler in #1270

Test files

https://burohappold.sharepoint.com/:f:/r/sites/BHoM/02_Current/12_Scripts/01_Test%20Scripts/BHoM_Engine/Data_Engine/Issue_1289_CreateTables?csf=1&e=KcLoUo

Changelog

  • Adding Create method for table class. One from List<IBHoMObject> and one from 2D table with header values.
  • Query method for getting table rows matching exact values in table
  • Query method for getting table rows matching values above set axis-values

Additional comments

@Martian42

@IsakNaslundBh IsakNaslundBh added the type:feature New capability or enhancement label Oct 29, 2019
@IsakNaslundBh IsakNaslundBh added this to the BHoM 3.0 β MVP milestone Oct 29, 2019
@IsakNaslundBh IsakNaslundBh self-assigned this Oct 29, 2019
@IsakNaslundBh IsakNaslundBh added the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Oct 29, 2019

/***************************************************/

[Description("")]
Copy link
Member

Choose a reason for hiding this comment

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

Are these going to be filled? If not, we can remove them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @epignatelli , yes, need to get all of these filled in.

/**** Public Methods ****/
/***************************************************/

[Description("")]
Copy link
Member

Choose a reason for hiding this comment

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

As above

[Description("")]
[Input("", "")]
[Output("", "")]
public static Table Table<T>(string name, string axis1Name, List<IComparable> axis1Values, string axis2Name, List<IComparable> axis2Values, List<List<T>> values, string valuesName = "Values")
Copy link
Member

Choose a reason for hiding this comment

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

What is an IComparable exactly? Can we input it from the UI side of things?
Not commenting all the rest, it's in other places as well.

@IsakNaslundBh
Copy link
Contributor Author

@adecler , would not mind a review from you on this, especially on the Serialiser_Engine part.

Copy link
Member

@adecler adecler left a comment

Choose a reason for hiding this comment

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

The serialiser looks fine to me.
I did a back and forth to and from Json and no data was lost.
See comment on the code though.

foreach (var kvp in properties)
{

columns.Add(new DataColumn(kvp.Key, kvp.Value.GetType()));
Copy link
Member

Choose a reason for hiding this comment

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

This needs a null check:

image

@IsakNaslundBh IsakNaslundBh removed the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Nov 8, 2019
@IsakNaslundBh
Copy link
Contributor Author

Pinging this!

Would anyone please be able to have a look at this PR so we can get it merged. Thanks!

@FraserGreenroyd
Copy link
Contributor

Sorry @IsakNaslundBh yes I'll try and get on this today!

@FraserGreenroyd
Copy link
Contributor

/azp run BHoM.CheckCore

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@FraserGreenroyd
Copy link
Contributor

/azp run BHoM_Engine.CheckCore

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@FraserGreenroyd
Copy link
Contributor

No pipelines are associated with this pull request.

Good to know, thanks bot 🤦‍♂

@al-fisher
Copy link
Member

No pipelines are associated with this pull request.

Good to know, thanks bot 🤦‍♂

@FraserGreenroyd it is the beginning of the end! 🤖
😆

Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

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

On the whole this looks ok from a code perspective - some comments in the code for tidiness as discussed @IsakNaslundBh .

Will run test scripts now.

Data_Engine/Query/Axes.cs Show resolved Hide resolved
Data_Engine/Create/Table.cs Show resolved Hide resolved
Data_Engine/Create/Table.cs Show resolved Hide resolved
Data_Engine/Create/Table.cs Outdated Show resolved Hide resolved
Data_Engine/Create/Table.cs Outdated Show resolved Hide resolved
Data_Engine/Create/Table.cs Outdated Show resolved Hide resolved
Data_Engine/Create/Table.cs Outdated Show resolved Hide resolved
@FraserGreenroyd
Copy link
Contributor

No pipelines are associated with this pull request.

Good to know, thanks bot 🤦‍♂

@FraserGreenroyd it is the beginning of the end! 🤖
😆

Skynet here we come... time to build a bunker! tinfoil hat firmly placed

@FraserGreenroyd
Copy link
Contributor

Test script worked with some modifications (I think the GH file was slightly out of date to the latest code, but replacing the components worked fine for me), so am happy to approve this PR when the above code comments are addressed @IsakNaslundBh 😄

@IsakNaslundBh
Copy link
Contributor Author

Thanks @FraserGreenroyd 🚀

Have fixed your white space comments. Also added a method to get all data out from the table. Added that method to the testscript as well and updated the outdated components.

Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

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

This LGTM @IsakNaslundBh - good to see where this grows 😄

@FraserGreenroyd
Copy link
Contributor

/azp run BHoM_Engine.CheckInstaller

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data_Engine: Add create and Query methods for Table
5 participants