Recommiting first API #2
Conversation
| self.build_rkt_solver(self.speciation_block, speciation_block=True) | ||
| self.build_gray_box(self.speciation_block) | ||
|
|
||
| self.build_rkt_state( |
There was a problem hiding this comment.
It looks like most of this is the same as what happens in the else branch (the default for speciation_block is False as far as I can tell).
Thus, I think you can make the if branch just for the speciation block, and then just run most of the else part for all cases. The only difference I see if whether speciation_block_built is True or not, which can be set in the if branch.
src/reaktoro_pse/reaktoro_block.py
Outdated
| speciation_block_built=False, | ||
| input_composition=None, | ||
| ): | ||
| """this will buld our rkt state specified block. |
There was a problem hiding this comment.
Typo: Capital to start doc string, "buld" -> "build".
Should "rkt" be expanded for later readers of the code?
src/reaktoro_pse/reaktoro_block.py
Outdated
|
|
||
| """ | ||
|
|
||
| """ defining which indexes shold be indexed |
There was a problem hiding this comment.
Typo: leading capital, indexes -> indices, shold -> should
src/reaktoro_pse/reaktoro_block.py
Outdated
| if specific input is set to be not Indexed the index will be set to None""" | ||
|
|
||
| index = self.index() | ||
| composition_indexed = index |
There was a problem hiding this comment.
An inline comment as to why these are needed might be helpful - my initial reaction was that they were unnecessary until I read the next part.
src/reaktoro_pse/reaktoro_block.py
Outdated
|
|
||
| convert_to_rkt_species = self.config.convert_to_rkt_species | ||
| pH = self.config.pH | ||
| if speciation_block == False: |
There was a problem hiding this comment.
Minor comment, but using and would make this only one line.
There was a problem hiding this comment.
fixed (legacy code...)
| ) | ||
| block.rkt_block_builder.build_reaktoro_block() | ||
|
|
||
| def display_jacobian_outputs(self): |
There was a problem hiding this comment.
Rather than a logger here, I would suggest allow the user to provide a stream object (generally a StringIO), and if not defaulting to stdout. This makes i ta bit easier to handle and test the output.
Also, looking at these I wonder if most of the code could be moved to the jacobian object and this method just be a wrapper around that (sort of like how it already calls rkt_jacobian.display_jacobian_output_types, but move all the code to there).
There was a problem hiding this comment.
Yeah that would make sense. I will look into it.
| else: | ||
| self.rkt_block_builder.set_user_jacobian_scaling(user_scaling_dict) | ||
|
|
||
| def initialize(self): |
There was a problem hiding this comment.
Is this equivalent to the existing process_block.initialize() method from IDAES?
If so, we should probably instead move to an Initializer object, as we plan to deprecate the model.initialize() API in the future. For backward compatibility, we can keep the initialize method, but have it just wrap the Initializer instead.
There was a problem hiding this comment.
I will put a todo for this.
|
Also, a quick link to the previous PR where I made some additional comments before it was closed: #1 |
Co-authored-by: Ludovico Bianchi <[email protected]>
|
The last commit includes final updates to API that adds support to liquid phases, condensed phases and solid phases. This also updates some of the structure to improve the extensibility of the API and adds a tutorial for combustion calculations that shows how databases can be modified and used with the ReaktoroBlock directly. After this point only minor API and tests fixes will be implemented. |
This is recommit of the main API.
Please review as needed.