Skip to content

Conversation

@matentzn
Copy link
Collaborator

@hrshdhgd can you deal with this release? I have done no tests etc, but there was a major change in the RDF serialiser (semapv namespace was wrong), so best run some tests?

@hrshdhgd
Copy link
Contributor

@matentzn : All tests pass!

Copy link
Collaborator Author

@matentzn matentzn left a comment

Choose a reason for hiding this comment

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

I approve of this, but see my comment for a clarifying question. You can merge at your own discretion.

@hrshdhgd
Copy link
Contributor

@matentzn : Changed the logic a little bit:

  • Earlier I was splitting the dataframe based on the value of predicate_modifier and acting separately.
  • Now, I act on the dataframe as a whole (Not and "" combined) while reconciling dataframes.

self.assertEqual(8, len(df1.index))
df2 = deal_with_negation(self.msdf2.df)
self.assertEqual(12, len(df2.index))
self.assertEqual(14, len(df2.index))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is what concerns me most, can you figure out what these two lines are - the update should not have change the number of rows in the reconciled part.

Copy link
Contributor

@hrshdhgd hrshdhgd Jun 15, 2023

Choose a reason for hiding this comment

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

Yeah now I remember. ..... The negative - positive splitting was because we wanted to enforce the rule of "negative trumps positive" in case of a confidence tie. Because we are doing this, grouping by predicate_modifier makes no sense. Hence KEY_FEATURES => TRIPLE_IDS.

Restore the major refactor to what it was before. Crisis averted, thanks to you!

@hrshdhgd
Copy link
Contributor

@matentzn : Ready again!

Copy link
Collaborator Author

@matentzn matentzn left a comment

Choose a reason for hiding this comment

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

approved (cant approve as its my PR)

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.

3 participants