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

FEAT - 328 - Add support for maximum specification link depth #341

Merged
merged 9 commits into from
Aug 24, 2023

Conversation

da1910
Copy link
Collaborator

@da1910 da1910 commented Aug 24, 2023

This PR adds maximum_specification_link_depth as a property on the BomAnalyticsClient object. Adds a pair of integration tests, and a couple of unit tests, as well as a new example cell in the notebooks.

@da1910 da1910 requested a review from Andy-Grigg August 24, 2023 09:27
@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2023

Codecov Report

Merging #341 (faaa3f9) into main (0f1737c) will decrease coverage by 0.08%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##             main     #341      +/-   ##
==========================================
- Coverage   95.32%   95.25%   -0.08%     
==========================================
  Files          10       10              
  Lines        1370     1390      +20     
==========================================
+ Hits         1306     1324      +18     
- Misses         64       66       +2     
Files Changed Coverage Δ
src/ansys/grantami/bomanalytics/_connection.py 88.63% <93.33%> (+0.24%) ⬆️

Copy link
Collaborator

@Andy-Grigg Andy-Grigg left a comment

Choose a reason for hiding this comment

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

Code looks fine, just some wording modifications.

def maximum_spec_link_depth(self) -> Optional[int]:
"""Limits the maximum depth of specification to specification links that will be followed when determining
impacted substances or compliance.
If None then no limit will be applied, this may lead to performance issues on databases with large numbers of
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
If None then no limit will be applied, this may lead to performance issues on databases with large numbers of
If None then no limit will be applied, which may lead to performance issues on databases with large numbers of


@property
def maximum_spec_link_depth(self) -> Optional[int]:
"""Limits the maximum depth of specification to specification links that will be followed when determining
Copy link
Collaborator

Choose a reason for hiding this comment

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

The wording here is really hard to get right. To be both precise and concise. How about:

"Limits the maximum number of specification-to-specification links that will be followed when processing a query. If specified, specification-to-specification links will be truncated at the specified depth, and only coatings and substances identified up to and including that point will be included in the analysis.

Defaults to None, which applies no limit to the number of specification-to-specification links. This may lead to performance issues if there are large numbers of specification-to-specification links present in the database.

Note: this limit applies to each branch of the BoM individually. This is not a global limit on the number of specification-to-specification links that will be traversed across the entire BoM, instead it is a limit on the maximum depth of specifications below any individual specification node."

src/ansys/grantami/bomanalytics/_connection.py Outdated Show resolved Hide resolved
src/ansys/grantami/bomanalytics/_connection.py Outdated Show resolved Hide resolved
src/ansys/grantami/bomanalytics/_connection.py Outdated Show resolved Hide resolved
# This package supports traversing specification to specification links, by default all such links will be followed.
# For typical databases this is the correct and desired behaviour, however in some circumstances this may cause query
# times and response sizes to become very large. In such cases you can control the number of nested links that will be
# followed using the ``maximum_spec_link_depth`` parameter on the ``BomAnalyticsClient`` object.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# followed using the ``maximum_spec_link_depth`` parameter on the ``BomAnalyticsClient`` object.
# using the ``maximum_spec_link_depth`` parameter on the ``BomAnalyticsClient`` object.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a Restructured Text xref in here, to link to the actual property documentation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Never mind, I realize this is in an example, and so any kind of link to the API documentation will probably be too fragile.

Co-authored-by: Andy Grigg <andrew.grigg@ansys.com>
@Andy-Grigg
Copy link
Collaborator

Andy-Grigg commented Aug 24, 2023

I realize we also need to add notes, at least on the property docstring, that explain this is only supported in the 2023 R2 RS reports bundle and later.

Copy link
Collaborator

@Andy-Grigg Andy-Grigg left a comment

Choose a reason for hiding this comment

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

Very minor changes

src/ansys/grantami/bomanalytics/_connection.py Outdated Show resolved Hide resolved
Co-authored-by: Andy Grigg <andrew.grigg@ansys.com>
@da1910 da1910 added this pull request to the merge queue Aug 24, 2023
Merged via the queue into main with commit 79e6e55 Aug 24, 2023
@Andy-Grigg Andy-Grigg linked an issue Aug 24, 2023 that may be closed by this pull request
@ludovicsteinbach ludovicsteinbach deleted the feat/add_support_for_spec_depth branch January 17, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for maximum specification depth
3 participants