Adding reification step4 #38

lute47lillo · 2022-07-20T21:55:09Z

Addressing issue #37 being step 4 of issue #5 .

jorgefandinno · 2022-07-21T17:24:43Z

It looks good. Now we have to apply this to reife the whole rule. For instance, the following test

Lines 60 to 63 in 6f81113

    
           def test_epistemic_atom(self): 
        
               self.assert_equal_program( 
        
                   parse_program(":- &k{a}."), ":- k_u(a). {k_u(a)} :- u(a)." 
        
               )

should now be

    def test_epistemic_atom(self):
        self.assert_equal_program(
            parse_program(":- &k{a}."), ":- k(u(a)). {k(u(a))} :- u(a)."
        )

This is the next step.

lute47lillo · 2022-07-21T18:27:23Z

It looks good. Now we have to apply this to reife the whole rule. For instance, the following test

eclingo/tests/test_reification.py

Lines 60 to 63 in 6f81113

def test_epistemic_atom(self):

self.assert_equal_program(

parse_program(":- &k{a}."), ":- k_u(a). {k_u(a)} :- u(a)."

)

should now be
    def test_epistemic_atom(self):
        self.assert_equal_program(
            parse_program(":- &k{a}."), ":- k(u(a)). {k(u(a))} :- u(a)."
        )
This is the next step.

Okay, I was thinking through it. In this case, do you want to make the call to parse_progrma function and do a reification there and then continue with the parse of the program, or will we need a new parse_program function that accounts for the reification steps?
Because, as it is, we also have the ":-" characters indicating that it is a rule, and if we were to reify the whole thing that is not accounted for in the first place

lute47lillo · 2022-07-23T02:10:09Z

@jorgefandinno While I'm looking into completing the reification as we talked, for which I'll post a couple comments since I have seen there are a couple ways of addressing it, I have gotten the 3 test_app.Test_Examples to fail.

I have not made any big change since the PR and you can also see what I have just started to change on the commits. But I cannot find the reason why they are failing now. Could you take a look into those? Maybe trying to see if they fail on your local machine too. Thank you

jorgefandinno · 2022-07-24T15:53:37Z

@jorgefandinno While I'm looking into completing the reification as we talked, for which I'll post a couple comments since I have seen there are a couple ways of addressing it, I have gotten the 3 test_app.Test_Examples to fail.

I have not made any big change since the PR and you can also see what I have just started to change on the commits. But I cannot find the reason why they are failing now. Could you take a look into those? Maybe trying to see if they fail on your local machine too. Thank you

Those are integration tests (they test the whole up instead of a single module).
It is strange that no other test fails. This means that there are no enough test for the part that fails.
My advice is to add more tests to the module you are modifying, until you find the bug.
Also, be sure reification is only used when the option is passed.

lute47lillo · 2022-07-24T18:12:53Z

@jorgefandinno While I'm looking into completing the reification as we talked, for which I'll post a couple comments since I have seen there are a couple ways of addressing it, I have gotten the 3 test_app.Test_Examples to fail.
I have not made any big change since the PR and you can also see what I have just started to change on the commits. But I cannot find the reason why they are failing now. Could you take a look into those? Maybe trying to see if they fail on your local machine too. Thank you

Those are integration tests (they test the whole up instead of a single module). It is strange that no other test fails. This means that there are no enough test for the part that fails. My advice is to add more tests to the module you are modifying, until you find the bug. Also, be sure reification is only used when the option is passed.

Okay, I figured it out. Apparently if you import something into a file like parser.py and then do not use it, the integration tests will fail. Why? That I do not know, but once I deleted them, the tests started to pass.

tests/test_reification.py

src/eclingo/parsing/transformers/parser_negations.py

lute47lillo

@jorgefandinno Please review this comments and let me know what should be the right approach, I'll made the changes based on what is expected. Thank you

tests/test_reification.py

lute47lillo · 2022-08-01T15:28:52Z

On top of that, I'm getting an error with flake 8, isort cannot import name SortImports. I'm pretty sure I haven't changed any req file. Do you know if flake8 has maybe been updated?

jorgefandinno · 2022-08-01T20:40:43Z

On top of that, I'm getting an error with flake 8, isort cannot import name SortImports. I'm pretty sure I haven't changed any req file. Do you know if flake8 has maybe been updated?

I am not aware of that. I pulled the content of lute/reificationStep4 and all test pass on my computer.
Do you refer to other branch?

lute47lillo · 2022-08-01T21:06:22Z

On top of that, I'm getting an error with flake 8, isort cannot import name SortImports. I'm pretty sure I haven't changed any req file. Do you know if flake8 has maybe been updated?

I am not aware of that. I pulled the content of lute/reificationStep4 and all test pass on my computer. Do you refer to other branch?

The tests are passing, but here on github when the checks are running, flak8 fails and that is why the checks are failing. If you peek details you'll see how the actual tests are all up to date and passing

This reverts commit 9aead79.

lute47lillo

@jorgefandinno

noxfile.py

jorgefandinno · 2022-08-29T22:56:05Z

Can you try the tests in your computer by running nox?

They fail in github and in my computer.
If I run them with coverage run -m unittest, they work.

It is very strange.

lute47lillo · 2022-08-29T23:05:10Z

Can you try the tests in your computer by running nox?

They fail in github and in my computer. If I run them with coverage run -m unittest, they work.

It is very strange.

I'm pretty sure that there's an error with the installation of the requirements. I was doing some testing using the AST test case class and running them with the command nox -r. And I saw this message and tried the command nox without the flag -r and now the tests are failing due to a coverage exit code -11.

It is indeed very strange

jorgefandinno · 2022-08-30T00:47:38Z

Can you try the tests in your computer by running nox?
They fail in github and in my computer. If I run them with coverage run -m unittest, they work.
It is very strange.

I'm pretty sure that there's an error with the installation of the requirements. I was doing some testing using the AST test case class and running them with the command nox -r. And I saw this message and tried the command nox without the flag -r and now the tests are failing due to a coverage exit code -11.

It is indeed very strange

This is solved. Coverage is not 100%. Can you look into it?

lute47lillo · 2022-08-30T15:20:57Z

Can you try the tests in your computer by running nox?
They fail in github and in my computer. If I run them with coverage run -m unittest, they work.
It is very strange.

I'm pretty sure that there's an error with the installation of the requirements. I was doing some testing using the AST test case class and running them with the command nox -r. And I saw this message and tried the command nox without the flag -r and now the tests are failing due to a coverage exit code -11.
It is indeed very strange

This is solved. Coverage is not 100%. Can you look into it?

Is there a way of running all tests without having to list them one by one on the noxfile session.run command?

Coverage will fail to achieve 100% if some functions are not being used for the specified tests

lute47lillo · 2022-08-30T15:29:35Z

@jorgefandinno Coverage is fixed now by running all tests

…ngAST Lute/reification step4 testing ast

lute47lillo · 2022-10-03T20:48:17Z

@jorgefandinno Please review the latest changes I have made, I believe it is all fixed now, and all the tests are passing.

jorgefandinno · 2022-10-05T19:44:51Z

src/eclingo/parsing/transformers/function_transformer.py

+from clingo.ast import Transformer
+
+
+def _rule_to_symbolic_term_adapter(rules):


Is this function used anywhere besides the tests? If not, the function should go to the test file.

Not for now, is the function I used as helper to call the transformer. It can be moved back to the tests

This is already solved and moved to the test file. @jorgefandinno

Ok, now I understand. It make sense to have a function that wraps the transformer. It is a good idea.
I got confused by the naming. The convention is to start by a underscore _ private functions that are only used in the module. The best idea would be to move the function back here renamed without the underscore. Prepend an underscore to the Transformer and outside of this file use only the function and never the transformer. Is that possible?

I moved it back and removed the underscore, but I do not understand the prepend an underscore to the Transformer, should I rename the Transformer class to be _? I have not seen any other Transformer being named like that

https://github.com/potassco/python-clingox/blob/4089afed444e9e6ddea9d03092e2903680f1d748/clingox/ast.py#L815

lute47lillo · 2022-10-14T20:34:36Z

@jorgefandinno The refactor issue should be solved now

Adding reification step4

6f81113

lute47lillo requested a review from jorgefandinno as a code owner July 20, 2022 21:55

lute47lillo added 2 commits July 22, 2022 20:49

dealing with aggregate reification

fa14ae5

Little typo

a7e703b

lute47lillo added 4 commits July 24, 2022 12:57

Reverting changes for app tests

ef78eee

Working tests

497df9f

MErging fixed branch

e450aa0

fixed failed test app

5f1278a

lute47lillo added 4 commits July 25, 2022 23:42

reification parameterized func

8ef7441

negation reified

94fb43c

Coverage

c6af7f3

Uncomment some tests

a5e6122

lute47lillo commented Jul 28, 2022

View reviewed changes

tests/test_reification.py Show resolved Hide resolved

src/eclingo/parsing/transformers/parser_negations.py Show resolved Hide resolved

Strong negation

58674f7

lute47lillo commented Jul 30, 2022

View reviewed changes

tests/test_reification.py Show resolved Hide resolved

tests/test_reification.py Outdated Show resolved Hide resolved

lute47lillo added 3 commits August 1, 2022 09:58

Removing redundant rule

c53b21c

Coverage

ab08d84

More coverage

d49fae7

lute47lillo added 3 commits August 2, 2022 17:57

Deleting flake8-isort from noxfile

9aead79

Revert "Deleting flake8-isort from noxfile"

7941abb

This reverts commit 9aead79.

Removing flake8-isort from noxfile

95a0e61

lute47lillo commented Aug 2, 2022

View reviewed changes

noxfile.py Outdated Show resolved Hide resolved

boolean trap

3f93d62

jorgefandinno force-pushed the lute/reificationStep4 branch from 45dec4a to 3f93d62 Compare August 29, 2022 22:51

some issues with version

f625974

lute47lillo added 2 commits August 30, 2022 10:26

Coverage fix

29a9993

remove txt files

cf79c0a

lute47lillo requested a review from jorgefandinno August 30, 2022 18:53

jorgefandinno and others added 8 commits August 31, 2022 14:28

update

beca7e1

SymbolicTerm transformer

cc4744e

Fix -a name issue on UnaryOperation

947e519

Sorting error fixed, more tests passing

facbdc1

Parameters test fixed, ensuring Interval SymbolicTerms

d1e7147

All tests fixed

61ba044

pylint

9bd917a

Merge pull request #39 from jorgefandinno/lute/reificationStep4_testi…

0775e59

…ngAST Lute/reification step4 testing ast

Fixed clingo transformer

2fe3894

jorgefandinno reviewed Oct 5, 2022

View reviewed changes

lute47lillo and others added 5 commits October 5, 2022 17:26

Function moved

73384c2

moved function wrap back

805bedb

underscore Transformer

6afab6d

refactor

f938084

Refactor

08fece6

lute47lillo closed this Mar 3, 2023

lute47lillo deleted the lute/reificationStep4 branch May 6, 2023 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding reification step4 #38

Adding reification step4 #38

lute47lillo commented Jul 20, 2022

jorgefandinno commented Jul 21, 2022

lute47lillo commented Jul 21, 2022

lute47lillo commented Jul 23, 2022

jorgefandinno commented Jul 24, 2022

lute47lillo commented Jul 24, 2022

lute47lillo left a comment

lute47lillo commented Aug 1, 2022

jorgefandinno commented Aug 1, 2022

lute47lillo commented Aug 1, 2022

lute47lillo left a comment

jorgefandinno commented Aug 29, 2022

lute47lillo commented Aug 29, 2022

jorgefandinno commented Aug 30, 2022

lute47lillo commented Aug 30, 2022

lute47lillo commented Aug 30, 2022

lute47lillo commented Oct 3, 2022

jorgefandinno Oct 5, 2022

lute47lillo Oct 5, 2022

lute47lillo Oct 5, 2022

jorgefandinno Oct 6, 2022

lute47lillo Oct 6, 2022

jorgefandinno Oct 6, 2022

lute47lillo commented Oct 14, 2022

		from clingo.ast import Transformer


		def _rule_to_symbolic_term_adapter(rules):

Adding reification step4 #38

Adding reification step4 #38

Conversation

lute47lillo commented Jul 20, 2022

jorgefandinno commented Jul 21, 2022

lute47lillo commented Jul 21, 2022

lute47lillo commented Jul 23, 2022

jorgefandinno commented Jul 24, 2022

lute47lillo commented Jul 24, 2022

lute47lillo left a comment

Choose a reason for hiding this comment

lute47lillo commented Aug 1, 2022

jorgefandinno commented Aug 1, 2022

lute47lillo commented Aug 1, 2022

lute47lillo left a comment

Choose a reason for hiding this comment

jorgefandinno commented Aug 29, 2022

lute47lillo commented Aug 29, 2022

jorgefandinno commented Aug 30, 2022

lute47lillo commented Aug 30, 2022

lute47lillo commented Aug 30, 2022

lute47lillo commented Oct 3, 2022

jorgefandinno Oct 5, 2022

Choose a reason for hiding this comment

lute47lillo Oct 5, 2022

Choose a reason for hiding this comment

lute47lillo Oct 5, 2022

Choose a reason for hiding this comment

jorgefandinno Oct 6, 2022

Choose a reason for hiding this comment

lute47lillo Oct 6, 2022

Choose a reason for hiding this comment

jorgefandinno Oct 6, 2022

Choose a reason for hiding this comment

lute47lillo commented Oct 14, 2022