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

Update US_SteelSection values with high diff #159

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Martian42
Copy link

@Martian42 Martian42 commented Jul 8, 2024

NOTE: Depends on

Issues addressed by this PR

Closes #157

Test files

https://burohappold.sharepoint.com/:f:/r/sites/BHoM/02_Current/12_Scripts/01_Issue/BHoM/BHoM_Datasets/%23157-Correct%20US%20steel%20section%20properties%20with%20larger%20than%202%20percent%20difference?csf=1&web=1&e=pyIwrm

Changelog

Additional comments

The inaccuracy is from the catalogue side. All values which is higher than 2% diff between BHoM and catalogue value has been updated to BHoM value.

Added @NikotaLitzin to review from a US perspective. Note that their is a v16.0 for AISC Shapes Databsase (this Dataset is based on v15.0). For the purpose of this PR lets review the shape profiles being modified, and then we can update to v16.0 seprately.

@Martian42 Martian42 self-assigned this Jul 8, 2024
@Martian42 Martian42 requested a review from Chrisshort92 July 8, 2024 14:53
@peterjamesnugent peterjamesnugent added the type:bug Error or unexpected behaviour label Jul 12, 2024
@peterjamesnugent peterjamesnugent added this to the BHoM 7.3 β MVP milestone Jul 12, 2024
@peterjamesnugent
Copy link
Member

@BHoMBot check dataset-compliance

Copy link

bhombot-ci bot commented Jul 12, 2024

@peterjamesnugent to confirm, the following actions are now queued:

  • check dataset-compliance

There are 24 requests in the queue ahead of you.

Copy link
Member

@peterjamesnugent peterjamesnugent left a comment

Choose a reason for hiding this comment

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

Happy to approve this - the /develop branch has nearly 300 properties above the 2% threshold - so great that this has been picked up to highlight errors in the source data. We should highlight this to the AISC so they can update their catalouges.

I do get some above 2% but on inspection the numbers are exactly the same, despite no rounding (and the numbers are small enough that differences are miniscule):
image

@peterjamesnugent
Copy link
Member

@BHoMBot check required

Copy link

bhombot-ci bot commented Jul 12, 2024

@peterjamesnugent to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer

@peterjamesnugent peterjamesnugent self-requested a review July 12, 2024 15:52
Copy link
Member

@peterjamesnugent peterjamesnugent left a comment

Choose a reason for hiding this comment

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

@Martian42, I believe you have serialised the Dataset with rounded values, can you reserialise the dataset without any rounding please so it is identical between the BHoMDataset and our Integration methods (otherwise we are just adding a %Diff for no reason).

image

@Martian42
Copy link
Author

@Martian42, I believe you have serialised the Dataset with rounded values, can you reserialise the dataset without any rounding please so it is identical between the BHoMDataset and our Integration methods (otherwise we are just adding a %Diff for no reason).

image

Thanks @peterjamesnugent, correctly formatted the BHoM values, test script no longer showing anything over 2% diff!
image

@peterjamesnugent
Copy link
Member

@BHoMBot check compliance

Copy link

bhombot-ci bot commented Jul 26, 2024

@peterjamesnugent to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance

@peterjamesnugent peterjamesnugent self-requested a review July 26, 2024 14:24
@peterjamesnugent
Copy link
Member

@BHoMBot check ready-to-merge

Copy link

bhombot-ci bot commented Jul 30, 2024

@peterjamesnugent to confirm, the following actions are now queued:

  • check ready-to-merge

There are 6 requests in the queue ahead of you.

@peterjamesnugent peterjamesnugent self-requested a review July 31, 2024 09:11
Copy link
Member

@peterjamesnugent peterjamesnugent left a comment

Choose a reason for hiding this comment

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

Happy changes have been made and values > 2% deviance have been updated. As mentioned earlier, worth reaching out to the source of the data to update their catalouge.

@peterjamesnugent
Copy link
Member

@BHoMBot check ready-to-merge

Copy link

bhombot-ci bot commented Jul 31, 2024

@peterjamesnugent to confirm, the following actions are now queued:

  • check ready-to-merge

@peterjamesnugent
Copy link
Member

@BHoMBot check required

Copy link

bhombot-ci bot commented Jul 31, 2024

@peterjamesnugent to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer

There are 4 requests in the queue ahead of you.

@peterjamesnugent
Copy link
Member

@BHoMBot check ready-to-merge

Copy link

bhombot-ci bot commented Jul 31, 2024

@peterjamesnugent to confirm, the following actions are now queued:

  • check ready-to-merge

There are 9 requests in the queue ahead of you.

@IsakNaslundBh
Copy link
Contributor

Before this is being merged, can it please be reviewd by a US engineer. I seem to remeber that when these dataset where first generated, this difference was found, but that the decision made back then was to keep the catalogue values, as that is what is the more defining metric for a particular section, rather than the exact dimensions down to the last root radius.

Before moving away from these catalogue values, I think that it needs to be confirmed by a US structural engineer with enough experience.

@IsakNaslundBh
Copy link
Contributor

For not to elaborate reference on the above, please see #5 (review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Error or unexpected behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BHoM_Datasets: US_SteelSection differences between calculated and extract serialized values
3 participants