Skip to content
This repository has been archived by the owner on Apr 23, 2021. It is now read-only.

Mixing io ii #23

Merged
merged 11 commits into from
Jun 30, 2015
Merged

Mixing io ii #23

merged 11 commits into from
Jun 30, 2015

Conversation

jmarcelogimenez
Copy link
Contributor

We have solved the integration between the file-IO wrapper with the internalInterfaces wrapper. Examples and Unittests are working.

@@ -0,0 +1,312 @@
""" test foam native interface
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is now for IO wrapper interface. Should be changed to use internal interface and add test of methods which are missing

Choose a reason for hiding this comment

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

The test has been changed to use the internal interface

@@ -0,0 +1,210 @@
/*---------------------------------------------------------------------------*\
Copy link
Member

Choose a reason for hiding this comment

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

There were no differences between this file LESModel.C and the one found in OpenFoam 2.2.x ("OpenFOAM-XXX/src/turbulenceModels/incompressible/LES/LESModel/LESModel.C"). Can this file be removed and the one from OpenFoam used?

Choose a reason for hiding this comment

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

Fixed

@@ -3,12 +3,12 @@
"""

from simphony.core.cuba import CUBA
Copy link
Member

Choose a reason for hiding this comment

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

@khiltunen, @jmarcelogimenez , the foam_controlwrapper/examples/poiseuille.py and foam_internalwrapper/examples/poiseuille.py are also mostly the same. Both types of wrappers support the same functionality, right? When looking at the files, it seems so..except for one minor thing (i.e. CUBAExt.NUMBER_OF_CORES).

I would think that these are examples of using simphony-openfoam (regardless of its its the INTERNAL or FILE-IO version). Therefore, foam_controlwrapper/examples/poiseuille.py and foam_internalwrapper/examples/poiseuille.py could be replaced by`examples/poiseuille.py``.

I would suggest not trying to tackle this in this PR but to address it in a seperate PR and an issue should be opened up now (e.g. "make a single poiseuille.py")

Copy link
Member

Choose a reason for hiding this comment

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

@jmarcelogimenez , this comment has been not addressed. An issue should be opened if it is not going to be addressed in this PR.

@nathanfranklin
Copy link
Member

The code is passing flake8 now.

@santiagomarquezd , are there some local changes you made that were not pushed to github yet?

@nathanfranklin
Copy link
Member

What is the status of this PR?

CI is now failing. See https://travis-ci.org/simphony/simphony-openfoam/builds/63707826

@khiltunen
Copy link
Contributor

@santiagomarquezd could you please let me know if assistance is needed to get this PR fixed

@santiagomarquezd
Copy link

Hi @khiltunen, we detected the same problem here in our local repository, we will work on that today

@jmarcelogimenez
Copy link
Contributor Author

This branch is almost ready to merge, excepting the test fails that ocurr when a cuds file is read. We are following the discussion in #26, so we are waiting for instructions about how to manage it.

@nathanfranklin
Copy link
Member

This branch is almost ready to merge, excepting the test fails that ocurr when a cuds file is read. We are following the discussion in #26, so we are waiting for instructions about how to manage it.

i would think just using a version of simphony-common 0.1.3 would do the trick at the moment (and not have to wait for #26)

Set that as the fixed version in requirements files and in the setup.py

@khiltunen
Copy link
Contributor

I agree that we should stick on 0.1.3 for now.

@nathanfranklin
Copy link
Member

I agree that we should stick on 0.1.3 for now.

@jmarcelogimenez , there is a PR (#29) under review so that the master now uses 0.1.3 of simphony-common.

Once that PR (#29) is merged, can you merge these new changes in master to this branch (see _What should I do if there is a new bugfix release that helps me with a feature PR?_ in the FAQ on https://publicwiki-01.fraunhofer.de/SimPhoNy-Project/index.php/Simphony_packages_development_workflow or let us know if you need assistance or want us to do it).

This should resolve the current error that revolves around the format of the file being used for testing.

@nathanfranklin
Copy link
Member

@jmarcelogimenez , master now uses 0.1.3 of simphony-common. Please merge these new changes in master to this branch (see my previous comment).

@nathanfranklin
Copy link
Member

Are there any updates on this PR?

@jmarcelogimenez
Copy link
Contributor Author

We have solved the simphony-common version issues. Waiting for travis checking ...

@@ -266,8 +266,8 @@ def test_run_time(self):
new_vel = cell.data[CUBA.VELOCITY]
new_pres = cell.data[CUBA.PRESSURE]

self.assertNotEqual(old_vel, new_vel)
self.assertNotEqual(old_pres, new_pres)
#self.assertNotEqual(old_vel, new_vel)
Copy link
Member

Choose a reason for hiding this comment

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

why are these commented out?

@jmarcelogimenez
Copy link
Contributor Author

@nathanfranklin we do not know why the travis test fail. In our local installations the tests work fine. If you look at this build: https://travis-ci.org/simphony/simphony-openfoam/builds/68353970, you can see the same result that we obtain here. Rarely, next travis builds do not work, without change the code! It looks like the test is reading an original OF file instead of our modificated file (fvSchemes.C). Then, if we can not reproduce that failure, we can not solve it.

Any idea?

Update: The problem was a linking precedence. Our compiler worked different than others, then the behavior was unpredictable. We have fixed the method selected in order to use our implementation.

@jmarcelogimenez
Copy link
Contributor Author

We have solved some linking issues.
This PR is able to be merged.
@nathanfranklin, will you do the merging?

@nathanfranklin
Copy link
Member

@khiltunen, have you had a look too to see if your comments were addressed?

@jmarcelogimenez , I added some issues to the comments to be addresed in later PRS. From my end it looks good to merge.

@khiltunen
Copy link
Contributor

My comment's were all addressed. So this is OK to merge

@jmarcelogimenez
Copy link
Contributor Author

So ... who is responsible for making the merge?

@nathanfranklin
Copy link
Member

@jmarcelogimenez , the developers of this repository. so you all and @khiltunen.

jmarcelogimenez added a commit that referenced this pull request Jun 30, 2015
@jmarcelogimenez jmarcelogimenez merged commit 590934b into master Jun 30, 2015
@@ -0,0 +1,665 @@
""" Mesh module
Copy link
Member

Choose a reason for hiding this comment

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

This file is duplicate with foam_internalwrapper/foam_mesh.py please remove

Copy link
Member

Choose a reason for hiding this comment

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

I will open a github issue

@nathanfranklin nathanfranklin deleted the mixing-io-ii branch July 1, 2015 15:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants