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 CAPI #11

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Update CAPI #11

wants to merge 13 commits into from

Conversation

schmoelder
Copy link
Contributor

@schmoelder schmoelder commented Jun 16, 2023

To-do

  • Implement CAPI Calls
    • Last State (waiting for changes upstream in CADET-Core)
    • Coordinates (waiting for changes upstream in CADET-Core)
  • Read solution
    • Implement all solution types
    • Some errors still come up when e.g. there is no solid phase in a unit. Need to investigate more
  • Formalize test in pipeline
  • Add tests for multiple particle types
  • Add tests for other unit operations
    • CSTR (currently not working with createLWE)
    • LRM
    • LRMP
  • Add tests for actual dimensions of arrays
  • Finalize CAPI in CADET-Core
    • Discuss dimensionality (see below)
    • Inconsistent flux parameters (see discussion below), pending 184.
    • Singleton dimension for particle_coordinates in lrmp should be squashed (imho)
    • Missing: Last sensitivity for each unit. Don't think we even have this for CADET-Core, right?
    • CSTR has singleton bulk dimension in C-API. Should be removed.
  • Pending a merge of Add autodetect cadetpath functionality from CADET-Process to CADET-Python #14 the tests should run in the CI.
  • Clean up hard-coded path to CADET-Core in test_dll.py (once v5.0 is released)
  • Add /root/meta
    • time_sim
    • file_format

Open questions

  • Do we distinguish between units that inherently cannot have ports and such that are can have ports? How are multi-port units treated when n_ports == 1? What about continuous discretization (e.g. radial coordinates) vs discontinuous ports (e.g. channels in MCT)?
  • Should we use n_units from the input data or from the API (e.g. in CadetDLLRunner.load_solution)?

Considerations for Interface v2

Moved to cadet/CADET-Core#123

@schmoelder
Copy link
Contributor Author

schmoelder commented Jun 20, 2023

I overhauled the CADETDll class and added missing methods. I also tried adding some tests which test different configurations that split components and/or ports.

@sleweke: Could we add an option in createLWE to allow setting multiple particle types?

@Immudzen: There is an issue when reading sensitivites for one of the examples in

https://github.com/modsim/CADET-Python/blob/ac294c2dfa02a3a42fdfd7666deb548800360d5d/cadet/cadet_dll_utils.py#L208

I get the following error:

Exception ignored on calling ctypes callback function: <function param_provider_get_string_array_item at 0x7f3d54930b80>
Traceback (most recent call last):
  File "/home/jo/code/CADET-Python/examples/../cadet/cadet_dll_utils.py", line 208, in param_provider_get_string_array_item
    reader.buffer = bytes_val
UnboundLocalError: local variable 'bytes_val' referenced before assignment

I assume it's because there is no else for the if-clauses.
Here, o is array([b'COL_POROSITY'], dtype=object). Any idea how to fix this?

@schmoelder
Copy link
Contributor Author

schmoelder commented Jun 27, 2023

As a quick fix, I did the following:

    if n in c:
        o = c[n]
        if index == 0:
            if hasattr(o, 'encode'):
                bytes_val = o.encode('utf-8')
            elif hasattr(o, 'decode'):
                bytes_val = o
            elif hasattr(o[index], 'decode'):
                bytes_val = o[index]
        else:
            if hasattr(o[index], 'encode'):
                bytes_val = o[index].encode('utf-8')
            elif hasattr(o[index], 'decode'):
                bytes_val = o[index]

It gets the job done but it doesn't make me very happy since it's more of a patch. I think I don't fully understand what's happening here so maybe we could give it some more structure?

Also, maybe we should still include some else, even if that means an exception that's makes it easier to understand what the issue is.

@schmoelder
Copy link
Contributor Author

@sleweke-bayer I worked a bit on the interface and I believe we can now read everything required for feature parity with the CLI. However, when writing tests, I realized that there are still some small issues regarding the dimensionality of the parameters (see description above). Once that's dealt with, I think we're ready to clean up and merge. 🤓

@schmoelder
Copy link
Contributor Author

schmoelder commented Mar 29, 2024

Some mismatch with solution_bulk detected (CLI returns shape=(nTime, nCol, nRad, nComp); CAPI returns shape=(nTime, nRad, nCol, nComp))

Expanding on this a little, I was again a bit confused what convention we finally used for structuring our multi-dimensional data. The only information I could find was in the Forum but this never seems to have been applied. From my understanding, we (mostly) use this structure:

bulk: 'nTime', ('nAxialCells',) ('nRadialCells' / 'nPorts',) 'nComp'
particle_liquid: 'nTime', ('nParTypes',) ('nAxialCells',) ('nRadialCells' / 'nPorts',) ('nParShells',) 'nComp'
particle_solid: 'nTime', ('nParTypes',) ('nAxialCells',) ('nRadialCells' / 'nPorts',) ('nParShells',) 'nComp', 'nBound'
flux: 'nTime', ('nParTypes',) ('nAxialCells',) ('nRadialCells' / 'nPorts',) 'nComp'

At least with this, I was able to read get consistent data shapes when comparing CLI and CAPI results. However, the flux was not consistent and would need to be restructured in CADET-Core (see 184).

If this is indeed correct, we should also update the forum post and publish this in other places s.t. we can easily access / find it.

@ronald-jaepel
Copy link
Collaborator

ronald-jaepel commented Apr 23, 2024

#14 is merged, I've rebased dll onto master to remove merge conflicts and pushed that branch to https://github.com/modsim/CADET-Python/tree/dll_rebased_onto_master to avoid force-pushes. Before merging dll into master, I suggest we merge it onto dll_rebased_onto_master and then merge from there.

@ronald-jaepel
Copy link
Collaborator

Once this is merged we should merge fau-advanced-separations/CADET-Process#169 to maintain compatibility.

warnings.warn("Deprecation warning: Support for setting cadet.cadet_path will be removed in a future version.")
warnings.warn(
"Support for setting cadet.cadet_path will be removed in a future version. "
"TODO: What alternative should be used?",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[TODO]: @ronald-jaepel I forgot, but what alternative should be used in the future?

cadet/cadet.py Show resolved Hide resolved
@schmoelder
Copy link
Contributor Author

schmoelder commented Aug 19, 2024

@hannahlanzrath Would you be up to adding the MCT in here?

Edit: Since the MCT is currently not available in createLWE, we have to wait until we update the testing structure in CADET-Core (see CADET-Core/#268).

@ronald-jaepel
Copy link
Collaborator

ronald-jaepel commented Aug 20, 2024

I'm getting this error:

Traceback (most recent call last):
  File "C:\Users\ronal\PycharmProjects\CADET-Python\cadet\cadet_dll.py", line 1697, in __del__
    self._api.deleteDriver(self._driver)
OSError: exception: access violation reading 0xFFFFFFFFFFFFFFFF

When

    def __del__(self) -> None:
        """
        Clean up the CADET driver on object deletion.
        """
        self._api.deleteDriver(self._driver)

is called.
Everything else works.

schmoelder and others added 4 commits August 23, 2024 13:56
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.

2 participants