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: apdl submodule #3385

Merged
merged 57 commits into from
Jan 22, 2025
Merged

feat: apdl submodule #3385

merged 57 commits into from
Jan 22, 2025

Conversation

clatapie
Copy link
Contributor

@clatapie clatapie commented Sep 5, 2024

This PR is the first one in order to automate the PyMAPDL _commands documentation.
The changes have been generated using pyconverter-xml2py.

This PR focus on the apdl submodule.

Pinging @ansys/pymapdl-developers for visibility. Feel free to provide any feedback on the way the docstrings and the source code generation are handled.

Note

This PR is meant to be merged within the feat/main_commands branch. The latter will gather all the submodule changes, one by one, prior to be merged to the main branch.

Checklist

@clatapie clatapie added documentation Documentation related (improving, adding, etc) enhancement Improve any current implemented feature labels Sep 5, 2024
@clatapie clatapie self-assigned this Sep 5, 2024
@clatapie clatapie requested a review from a team as a code owner September 5, 2024 16:03
@clatapie clatapie requested review from germa89 and pyansys-ci-bot and removed request for a team September 5, 2024 16:03
@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

@clatapie clatapie requested a review from RobPasMue September 5, 2024 16:04
@github-actions github-actions bot added new feature Request or proposal for a new feature and removed documentation Documentation related (improving, adding, etc) labels Sep 5, 2024
@germa89
Copy link
Collaborator

germa89 commented Sep 5, 2024

I am setting this PR as draft because I think it is still not ready until you fix the line length thing.

Ping us and switch this PR to non-draft when you feel it is ready for it.

@germa89 germa89 marked this pull request as draft September 5, 2024 16:06
@clatapie
Copy link
Contributor Author

clatapie commented Sep 5, 2024

I can rework on it if docstring length is a priority. I already fixed some issues related in ansys/pyconverter-xml2py#222 but not all of them as you can see.
Except from this issue, feel free to comment any other change you would like me to work on.

Also, I had to comment the codespell pre-commit hook because it was failing even when I was ignoring the dedicated directory ``./src/ansys/mapdl/core/_commands). The next commit will show it.

@github-actions github-actions bot added CI/CD Related with CICD, Github Actions, etc examples Publishing PyMAPDL examples documentation Documentation related (improving, adding, etc) dependencies maintenance General maintenance of the repo (libraries, cicd, etc) labels Oct 18, 2024
@wiz-inc-572fc38784
Copy link
Contributor

wiz-inc-572fc38784 bot commented Oct 18, 2024

Wiz Scan Summary

Scanner Findings
Vulnerability Finding Vulnerabilities
Data Finding Sensitive Data
Secret Finding Secrets
IaC Misconfiguration IaC Misconfigurations
Total

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

@clatapie clatapie force-pushed the feat/apdl_submodule branch from ac7b46a to 04dac45 Compare October 18, 2024 12:15
@github-actions github-actions bot removed CI/CD Related with CICD, Github Actions, etc examples Publishing PyMAPDL examples documentation Documentation related (improving, adding, etc) dependencies maintenance General maintenance of the repo (libraries, cicd, etc) labels Oct 18, 2024
@github-actions github-actions bot added the CI/CD Related with CICD, Github Actions, etc label Oct 18, 2024
@clatapie
Copy link
Contributor Author

clatapie commented Jan 9, 2025

A warning feature has been added in case some functions need specific warning at the top of its docstring.
Here is an example of how it can be used:
image

@github-actions github-actions bot removed the CI/CD Related with CICD, Github Actions, etc label Jan 9, 2025
@germa89
Copy link
Collaborator

germa89 commented Jan 14, 2025

@RobPasMue @MaxJPRey @clatapie since the MAPDL commands are pretty stable, I do not think we will be expecting many changes in the custom functions, so I would say that priorities should be:

  • MAPDL API compatibility (MAPDL users feedback)
  • Users feedback (PyMAPDL users feedback)
  • Translator module support
  • Maintainability

There is always time to break the API, I guess?

@RobPasMue
Copy link
Member

@RobPasMue @MaxJPRey @clatapie since the MAPDL commands are pretty stable, I do not think we will be expecting many changes in the custom functions, so I would say that priorities should be:

  • MAPDL API compatibility (MAPDL users feedback)
  • Users feedback (PyMAPDL users feedback)
  • Translator module support
  • Maintainability

There is always time to break the API, I guess?

I might be missing something. What are you proposing with this priority list? I think I got lost somewhere...

BTW - I do not agree on having Maintainability the last element of a priority list anyway 😄 maintainability should be at the core of whatever we decide, because it will make our lives easier in the future.

@germa89
Copy link
Collaborator

germa89 commented Jan 14, 2025

(I was still building on my previous comment, I hit enter too fast)

I think we have a choice to make here... maintainability (leave everything as the converter gives it) or user experience (better docs, better arguments handling). While I have always priorised both, I rarely had to choose one or another. So I think for this specific context, I am priorizing the list of items in my previous comment.

I do understand that PyAnsys core team might be more interested in maintainability because you manage way too many things. But I think PyMAPDL developers should focus on MAPDL and PyMAPDL users and their user experience, and since the MAPDL commands are not expected to change much, I rather do the initial work in detrimental of automation.

I am still not sure what is the best as the * methods breaks the translator module and the current API. So I think we should go for custom functions (even if it has more work).

I would love to see something like this for SECMODIF:

def secmodif(self, secid, keyword, nx="", ny="", nz="", kcn="", name="", name_new="", option="", dispPnltVal="", RotPnltVal="", **kwargs):
    """Modifies a pretension section

    APDL Command: SECMODIF

    This command have specific signature depending on the values of the given
    arguments.
    - If `Keyword = NORM`:
        SECMODIF,SECID, NORM, NX, NY, NZ, KCN
    - If `Keyword = NAME`:
        SECMODIF,SECID, NAME, Name
    - If `Keyword = JOIN`:
        SECMODIF,SECID, JOIN, Option, dispPnltVal, RotPnltVal

    Parameters
    ----------
    secid
        Unique section number. This number must already be assigned to a
        section.

    norm
        Keyword specifying that the command will modify the pretension
        section normal direction.

    nx, ny, nz
        Specifies the individual normal components to modify.

    kcn
        Coordinate system number. This can be either 0 (Global Cartesian),
        1 (Global Cylindrical) 2 (Global Spherical), 4 (Working Plane), 5
        (Global Y Axis Cylindrical) or an arbitrary reference number
        assigned to a coordinate system.

    name
        Change the name of the specified pretension section.
    
    name_new
        The new name to be assigned to the pretension section.

    join
        Set command actions to apply to joint sections only.
    
    option
        PNLT -- Modify penalty factors for the specified section.
    
    dispPnltVal
        Penalty value for displacement-based constraints:
        - `> 0` -- The number is used as a scaling factor to scale the
            internally calculated penalty values.
        - `< 0` -- The absolute value of the number is used as the penalty
            factor in calculations.
    
    RotPnltVal
        Penalty value for rotation-based constraints.
        - `> 0` -- The number is used as a scaling factor to scale the
            internally calculated penalty values.
        - `< 0` -- The absolute value of the number is used as the penalty
            factor in calculations.


    Notes
    -----
    The SECMODIF command either modifies the normal for a specified
    pretension section, or changes the name of the specified pretension
    surface.
    """
    # Sanity checks
    if keyword.upper() not in ["NORM", "NAME", "JOIN"]:
        raise ValueError(f"The given argument 'keyword' ({keyword}) is not valid.")

    if keyword  == "NORM":
        cmd = f"SECMODIF, {secid}, NORM, {nx}, {ny}, {nz}, {kcn}"
    elif keyword  == "NAME":
        cmd = f"SECMODIF, {secid}, NAME, {name or nx}, {name_new or ny}"
    elif keyword == "JOIN":
        cmd = f"SECMODIF, {secid}, JOIN, {option or nx},{dispPnltVal or ny},{RotPnltVal or nz}"
    else:
        raise ValueError("We couldn't map the arguments given....")
    
    self.run(cmd, **kwargs)

The doc string is slightly modified, just adding some info about the signatures in the extended summary part (before parameters).
The code is bit more complex, but I do believe it won't change much, and it can be included in a wrapped function using functools.wrap, this way it wont be much affected by docstring changes.

@RobPasMue
Copy link
Member

I would leave it up to @clatapie to decide since she is doing the work mostly, but for me I would keep any kind of custom function to the minimum (i.e. reduce their number as much as possible). Mostly because they will go outdated and it will be on the maintainers to keep them up to date.

The whole point of automating the API generation process (which was the initial purpose of all these efforts) was to free up time for maintainers and developers to work on actual problems inside PyMAPDL. Not to generate more work on the long run IMO.

In any case, we need to make progress with this. We have been iterating for too long and we don't have anything merged or close to be merged yet. We should be doing more iterations rather than trying to get it 100% right in the first shot.

@github-actions github-actions bot added the CI/CD Related with CICD, Github Actions, etc label Jan 22, 2025
@clatapie clatapie requested a review from germa89 January 22, 2025 11:11
@clatapie clatapie merged commit 2a15307 into feat/main_commands Jan 22, 2025
38 checks passed
@clatapie clatapie deleted the feat/apdl_submodule branch January 22, 2025 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD Related with CICD, Github Actions, etc dependencies documentation Documentation related (improving, adding, etc) enhancement Improve any current implemented feature maintenance General maintenance of the repo (libraries, cicd, etc) new feature Request or proposal for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants