Skip to content

FEAT: emit_schematic module is added. #6240

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ramin4667
Copy link
Contributor

Description

This PR adds a schematic feature to the EMIT project to create and connect components.

Issue linked

NA

Checklist

  • I have tested my changes locally.
  • I have added necessary documentation or updated existing documentation.
  • I have followed the coding style guidelines of this project.
  • I have added appropriate tests (unit, integration, system).
  • I have reviewed my changes before submitting this pull request.
  • I have linked the issue or issues that are solved by the PR if any.
  • I have agreed with the Contributor License Agreement (CLA).

@ramin4667 ramin4667 requested a review from a team as a code owner June 6, 2025 17:24
@ansys-reviewer-bot
Copy link
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

@ramin4667 ramin4667 requested a review from jsalant22 June 6, 2025 17:24
@github-actions github-actions bot added the enhancement New features or code improvements label Jun 6, 2025
@ramin4667 ramin4667 changed the title emit_schematic module is added. FEAT: emit_schematic module is added. Jun 6, 2025
Copy link

codecov bot commented Jun 6, 2025

Codecov Report

Attention: Patch coverage is 23.07692% with 60 lines in your changes missing coverage. Please review.

Project coverage is 85.14%. Comparing base (0760ca1) to head (53b6417).

❌ Your patch check has failed because the patch coverage (23.07%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6240      +/-   ##
==========================================
- Coverage   85.21%   85.14%   -0.07%     
==========================================
  Files         170      171       +1     
  Lines       64329    64407      +78     
==========================================
+ Hits        54820    54842      +22     
- Misses       9509     9565      +56     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jsalant22
Copy link
Contributor

@ramin4667 This is going to cause some merge conflicts with our main feature branch, mainly the new unit tests that you added. Some of the comments I made are also dependent on that branch. We should try to rebase this off that branch and then merge into that branch, instead of main, if its not too difficult. If it's a pain we can merge this to main and then address any comments dependent on that branch separately.

cc @bryankaylor

@jsalant22
Copy link
Contributor

Codecov Report

Attention: Patch coverage is 20.77922% with 61 lines in your changes missing coverage. Please review.

Project coverage is 81.60%. Comparing base (2c557ba) to head (bcdafb3).

❌ Your patch check has failed because the patch coverage (20.77%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
🚀 New features to boost your workflow:

@ramin4667 Are you able to run the coverage analysis locally? The main repo is still running 25.1 so all of your tests are basically skipped. If you haven't done this before, @bryankaylor should be able to help.

self.emit_instance = emit_instance

@property
def emit_com_module(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def emit_com_module(self):
def _emit_com_module(self):

We don't really want users accessing this directly, so let's make this method private. Doesn't prevent them from accessing it, but at least discourages its use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied.


@pyaedt_function_handler
def create_component(self, component_type, name=None, library=None):
"""Create a component using the EmitCom module.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Create a component using the EmitCom module.
"""Create a component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied.

component_type : str
Type of the component to create.
name : str, optional
Name of the component to create. Defaults to an empty string if not provided.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Name of the component to create. Defaults to an empty string if not provided.
Name of the component to create. AEDT defaults used if not provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied.

if not component_type:
raise ValueError("The 'component_type' argument is required.")

name = name or ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Never knew you could do this, nice!

component = next((comp for comp in matching_components if comp.name == component_type), None)
if not component:
self.emit_instance.logger.error(
f"Multiple components found for type '{component_type}', but no exact match. Please specify a unique component."
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this! Would be even better to list the components found so users can choose the option they want.

self.aedtapp = add_app(project_name="create_component", application=Emit)
self.aedtapp.logger.info = MagicMock()
new_radio = self.aedtapp.schematic.create_component("MICS")
assert new_radio == 3
Copy link
Contributor

Choose a reason for hiding this comment

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

You should double check with Apollo, but I don't think the node ID values are guaranteed. If nodes are somehow created in a different order, the node ID could change.

Granted, if you update to return the node, then you can instead assert new_radio.name = 'MICS' (or whatever it's supposed to be).

"Failed to create component of type 'lte': Multiple components found for type 'lte', but no exact match."
) in str(e.value)

@pytest.mark.skipif(config["desktopVersion"] < "2026.1", reason="Skipped on versions earlier than 2026 R1.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@pytest.mark.skipif(config["desktopVersion"] < "2026.1", reason="Skipped on versions earlier than 2026 R1.")
@pytest.mark.skipif(config["desktopVersion"] < "2025.2", reason="Skipped on versions earlier than 2025 R2.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected.

Comment on lines +1336 to +1337
assert new_radio == 3
assert new_antenna == 4
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert new_radio == 3
assert new_antenna == 4
assert new_radio == 3
assert new_antenna == 4

Same comment as above

assert new_antenna == 4
with pytest.raises(RuntimeError) as e:
self.aedtapp.schematic.create_radio_antenna("WrongComponent", "Radio", "Antenna")
assert "Failed to create radio of type 'WrongComponent'" in str(e.value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any error cases to test for create_radio_antenna?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already a test :
with pytest.raises(RuntimeError) as e:
self.aedtapp.schematic.create_radio_antenna("WrongComponent", "Radio", "Antenna")
assert "Failed to create radio of type 'WrongComponent'" in str(e.value)

@pytest.mark.skipif(config["desktopVersion"] < "2026.1", reason="Skipped on versions earlier than 2026 R1.")
def test_26_create_component(self, add_app):
self.aedtapp = add_app(project_name="create_component", application=Emit)
self.aedtapp.logger.info = MagicMock()
Copy link
Contributor

Choose a reason for hiding this comment

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

@bryankaylor Never seen this before, but maybe it can help us with the general node unit testing?

new_radio = self.aedtapp.schematic.create_component("MICS")
assert new_radio == 3
self.aedtapp.logger.info.assert_called_with(
rf"Using component 'MICS' from library 'Radios\Commercial Unlicensed Systems\Medical' for type 'MICS'."
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't need to be an f-string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected.

new_radio = self.aedtapp.schematic.create_component("MICS")
new_antenna = self.aedtapp.schematic.create_component("Antenna")
self.aedtapp.schematic.connect_components(new_radio, new_antenna)
self.aedtapp.logger.info.assert_called_with(f"Successfully connected components 'MICS' and 'Antenna'.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Just make this a normal string, not an f-string (codacy analysis is complaining)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected.


import pytest

import ansys.aedt.core
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove? Codacy claims its unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features or code improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants