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

Modified Unit-tests so that they always report scores using '.' as decimal separator #2

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

Conversation

Erechtheus
Copy link

Unit-tests do not work with German Locale (DE), because scores are here reported as 1,000 instead of 1.000. However, the tests expect English-locale to work, which uses decimal separator. I therefore slightly modified the tests so that the locale is set to EN/US.

@aalbahem
Copy link
Contributor

aalbahem commented Jan 4, 2024

Thanks for your contributions. I never knew German uses "," to denote decimal places, great to know that. I will try to have a look at the changes.

@Erechtheus
Copy link
Author

Yes, the "," decimal point thing is a real nuisance. :-(

@Erechtheus
Copy link
Author

Hi! I just realized that I messed up my pull request. I think the first changes are already sufficient to avoid the Unit-test errors:
9b5d6f5

Copy link
Contributor

@aalbahem aalbahem left a comment

Choose a reason for hiding this comment

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

@Erechtheus Thanks again for your contribution.

Though I acknowledge the problem you are trying to fix and it shows the limitation of of brateval core. It is too rigid and does not allow customisation.

I think the solution is not to replace it with Output.csv, the solution is to refactor the code so we can pass the formatter and load it at run time while having a default one. This way, if someone else wants another formatter, they do not need to change inside the code, but use brateval as a dependency and use their formatter in runtime. I am working on this right now, it would create a better way for everyone to use and contribute more formatter.

@@ -45,14 +50,14 @@ static void report(int level, String rt, int TP, int FP, int FN, int MFP, int MF
{ f_measure = (2*precision*recall)/(double)(precision+recall); }

System.out.println(rt
+ "|tp:" + TP
Copy link
Contributor

Choose a reason for hiding this comment

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

That will create incompatibility with any code that uses or expects this format. We use brateval internally in a project and I can assure this will break out code.

Nevertheless, I know what is the problem. The problem is that our code does not allow an easy way to override output format. I am going to create a new issue to track this.

@@ -26,12 +26,13 @@ public enum OutFmt {
}


public OutFmt outFmt = OutFmt.PLAIN;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is another breaking changes from what we have.

@aalbahem
Copy link
Contributor

This can be done better when #3 is done.

@Erechtheus
Copy link
Author

Dear @aalbahem , I completely agree with your comment. My merge request was unfortunately messed up. In case you are interested in fixing only the Locale-problem (and not the CSV part; which was not intended to be part of my merge request) you could have a look at the fixes here:

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