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

Added Complex Class #104

Merged
merged 21 commits into from
Apr 1, 2022
Merged

Added Complex Class #104

merged 21 commits into from
Apr 1, 2022

Conversation

ConduitDan
Copy link
Collaborator

@ConduitDan ConduitDan commented Mar 3, 2022

This change adds the complex number class to morpho complete with all the associated operations

complex.c and complex.h contain the bulk of the changes as they define the class

I have also changed vm.c to check if objects can be raised to powers the same way it checks if objects can be added or subtracted.

This change fixes #103 as well (I was looking at matrix as a template for this class)

Tests are all in one file test/complex/ComplexBasics.morpho

The change to test.py is so github now properly marks failing tests.

@ConduitDan
Copy link
Collaborator Author

I will also be adding doc, an example and a parsing pattern to identify im as an imaginary number.

Copy link
Collaborator

@joshichaitanya3 joshichaitanya3 left a comment

Choose a reason for hiding this comment

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

This looks great! Really satisfying that E^Complex(0,Pi) + 1 can be done.

I have added a couple of minor comments, but looks otherwise good to go.

c->real = value;
}
/** @brief Sets the imaginary part of a complex number.*/
void complex_setimag(objectcomplex *c, double value) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps setreal and setimag might be useful methods to have for the end user as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps, but I think that can be done with assignment and .real() and .imag()

for example
var A = Complex(1,1)
var A = Complex(2,A.imag())

print E^Complex(0,Pi) + 1
// expect: 0 + 0im


Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add tests for the real and imag methods?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, I'll add those

@joshichaitanya3
Copy link
Collaborator

I just realized that we will need to get the builtin math functions to work with complex numbers too, but that can be a separate update I suppose.

image

@softmattertheory
Copy link
Contributor

Checks are not passing...

@ConduitDan
Copy link
Collaborator Author

I just realized that we will need to get the builtin math functions to work with complex numbers too, but that can be a separate update I suppose.

I've actually got these working now, I'll push the new commit as soon I as have the tests written

@ConduitDan
Copy link
Collaborator Author

Checks are not passing...

This is due to issue #107 (and a precision mismatch from functionals/gradsq/gradsq3d.morpho)

I'll update gradsq3d to pass.

as for the too many args test (issue #107) I think the right thing to do is comment out that test (assume fail it) and when we fix #107 we can put it back online

@ConduitDan
Copy link
Collaborator Author

Also: I was thinking of adding real and imag as builtin functions instead of methods on complex numbers. so we don't have some functions that are builtin and some that are methods

@ConduitDan
Copy link
Collaborator Author

Hmmmm its clear this need a little more bake time, I'll pull out the test and bug fixes and put those in another pull request

@softmattertheory
Copy link
Contributor

This is due to issue #107 (and a precision mismatch from functionals/gradsq/gradsq3d.morpho)

I'll update gradsq3d to pass.

I partially updated gradsq3d.morpho so you should just reduce the comparison threshold on one of the tests.

as for the too many args test (issue #107) I think the right thing to do is comment out that test (assume fail it) and when we fix #107 we can put it back online

Ah yes; sorry I'd forgotten about this! I'll take a look at that one with valgrind.

@softmattertheory
Copy link
Contributor

softmattertheory commented Mar 4, 2022

Also: I was thinking of adding real and imag as builtin functions instead of methods on complex numbers. so we don't have some functions that are builtin and some that are methods

Maybe look at what Julia and other languages do [http://rigaux.org/language-study/syntax-across-languages.html is my goto!]? I also think that having re() and im() as functions and .real and .imag as methods would be ok too. Very often we want to write something like

re(z) + im(z)^2  

which is more compact and intuitive than

z.real() + z.imag()^2 

I think having both tho is ok because we want the object method API to be complete but we see re() and im() [or Re() and Im()?] as shortcuts for readability.

Hopefully this is helpful!

@ConduitDan
Copy link
Collaborator Author

ConduitDan commented Mar 10, 2022

OK! Latest update now add builtin function support for complex numbers (sin cos tan abs log ...) as well as adding 4 new builtin functions (conj, angle, real, imag) for complex numbers.

I have also added a parse pattern for detecting im. This means you can now enter complex numbers like
var A = 1+1im

I have also added an equality for complex numbers:
var B = 1+im
print A==B \\ expect: true

I stuck to the standard Juila uses for complex numbers https://docs.julialang.org/en/v1/manual/complex-and-rational-numbers/

I still have to finish the test suite before I submit

Left to do is for testing is

  • builtins
  • equality
  • im parse pattern
  • Sync with main to fix failing tests

@ConduitDan ConduitDan changed the title Added Complex Class, fixed a matrix bug Added Complex Class Mar 11, 2022
@ConduitDan
Copy link
Collaborator Author

Ok! Tests are written and some more bugs were fixed.

I think this is ready for full review and pull.
I just had one question about the need to bind or not in common.c:35 (I'm casting a float to a complex and leaving it unbound because its not needed beyond the scope of the function)

Please let me know what you think!

The writing the class itself was very easy and I plan on creating morpho class template. This change set is so far reaching because of the need for the new parse pattern, the inclusions of the builtin functions and the equality of complex number with 0 imaginary part and real numbers (I did not add other comparisons for these).

I think the next extension for complex numbers should be to expand the Matrix class to handle Complex Matrices. That will take some further thought and planning.

@softmattertheory
Copy link
Contributor

This is really exciting @ConduitDan! I'll review next week when I get back from March meeting.

If an object is needed within a C function, but not beyond, I recommend creating it on the C stack rather than creating/freeing. See MORPHO_STATIC_MATRIX and MORPHO_STATIC_STRING and how they're used.

Objects only need to be bound if they're returned to the runtime and must be disposed of when no longer referred to; anything that is tracked separately or is a temporary object doesn't need to be bound [this actually is a big benefit of the binding mechanism].

@joshichaitanya3
Copy link
Collaborator

The new changes look great! I like the ease that im provides, and I also like how real and imag are both builtin as well as member functions.

@ConduitDan
Copy link
Collaborator Author

ConduitDan commented Mar 29, 2022

  • Change file names to cmplx
  • Make macro to create a static complex number and use that in common.c (look in object.h:311)
  • Change common.c to cast to double complex (c-type) and compare

@softmattertheory
Copy link
Contributor

softmattertheory commented Mar 30, 2022

Clang threw a warning on this function at line 599. Previously, the results of atan2 were not being assigned to val. @ConduitDan check this is ok. Was there a test to lock all these methods down?

value Complex_angle(vm *v, int nargs, value *args) {
    objectcomplex *a=MORPHO_GETCOMPLEX(MORPHO_SELF(args));
    double val=atan2(cimag(a->Z),creal(a->Z));
    return MORPHO_FLOAT(val);
}

I've changed this in my update; just confirm this is what was intended.

@softmattertheory
Copy link
Contributor

@ConduitDan it's also worth documenting that if you add a new token type [as you do here] that you also MUST add an entry in cli.c for syntax highlighting. The syntax highlighter reuses the lexer and highlights by token type.

Fixed minor issues (incl a few warnings) in complex.

Syntax highlighting for complex numbers corrected.
@softmattertheory
Copy link
Contributor

Hmmm.. ComplexTrig.morpho and ComplexBuiltin.morpho are failing on macOS. I'll investigate.

Comparisons now made through abs rather than equality to work on macOS
@ConduitDan
Copy link
Collaborator Author

ConduitDan commented Mar 31, 2022

Clang threw a warning on this function at line 599. Previously, the results of atan2 were not being assigned to val. @ConduitDan check this is ok. Was there a test to lock all these methods down?

value Complex_angle(vm *v, int nargs, value *args) {
    objectcomplex *a=MORPHO_GETCOMPLEX(MORPHO_SELF(args));
    double val=atan2(cimag(a->Z),creal(a->Z));
    return MORPHO_FLOAT(val);
}

I've changed this in my update; just confirm this is what was intended.

Ooooh good catch, I missed the test for that method. I've added them in.
I've also updated this function to use complex_angle instead of doing an atan2, no reason that .angle() and angle(...) should use different numerics

@ConduitDan
Copy link
Collaborator Author

Ok, with this most recent change I have

  • Added a static complex macro
  • added sqrt(negitive#) (and test)
  • Cleaned up up equality
  • Renamed files to cmplx
  • Cleaned up and Added test for Complex.angle()

@softmattertheory
Copy link
Contributor

I think this is good to go and will merge later today!

@softmattertheory softmattertheory merged commit fc116c8 into main Apr 1, 2022
@ConduitDan ConduitDan deleted the ComplexNumbers branch April 13, 2022 14:05
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.

[Bug] Internal Consistency Error Matrix Right Addition
3 participants